net: openvswitch: fix middle attribute validation in push_nsh() action

[PATCH net] net: openvswitch: fix middle attribute validation in push_nsh() action

Posted by Ilya Maximets 2 weeks, 1 day ago

The push_nsh() action structure looks like this:

 OVS_ACTION_ATTR_PUSH_NSH(OVS_KEY_ATTR_NSH(OVS_NSH_KEY_ATTR_BASE,...))

The outermost OVS_ACTION_ATTR_PUSH_NSH attribute is OK'ed by the
nla_for_each_nested() inside __ovs_nla_copy_actions().  The innermost
OVS_NSH_KEY_ATTR_BASE/MD1/MD2 are OK'ed by the nla_for_each_nested()
inside nsh_key_put_from_nlattr().  But nothing checks if the attribute
in the middle is OK.  We don't even check that this attribute is the
OVS_KEY_ATTR_NSH.  We just do a double unwrap with a pair of nla_data()
calls - first time directly while calling validate_push_nsh() and the
second time as part of the nla_for_each_nested() macro, which isn't
safe, potentially causing invalid memory access if the size of this
attribute is incorrect.  The failure may not be noticed during
validation due to larger netlink buffer, but cause trouble later during
action execution where the buffer is allocated exactly to the size:

 BUG: KASAN: slab-out-of-bounds in nsh_hdr_from_nlattr+0x1dd/0x6a0 [openvswitch]
 Read of size 184 at addr ffff88816459a634 by task a.out/22624

 CPU: 8 UID: 0 PID: 22624 6.18.0-rc7+ #115 PREEMPT(voluntary)
 Call Trace:
  <TASK>
  dump_stack_lvl+0x51/0x70
  print_address_description.constprop.0+0x2c/0x390
  kasan_report+0xdd/0x110
  kasan_check_range+0x35/0x1b0
  __asan_memcpy+0x20/0x60
  nsh_hdr_from_nlattr+0x1dd/0x6a0 [openvswitch]
  push_nsh+0x82/0x120 [openvswitch]
  do_execute_actions+0x1405/0x2840 [openvswitch]
  ovs_execute_actions+0xd5/0x3b0 [openvswitch]
  ovs_packet_cmd_execute+0x949/0xdb0 [openvswitch]
  genl_family_rcv_msg_doit+0x1d6/0x2b0
  genl_family_rcv_msg+0x336/0x580
  genl_rcv_msg+0x9f/0x130
  netlink_rcv_skb+0x11f/0x370
  genl_rcv+0x24/0x40
  netlink_unicast+0x73e/0xaa0
  netlink_sendmsg+0x744/0xbf0
  __sys_sendto+0x3d6/0x450
  do_syscall_64+0x79/0x2c0
  entry_SYSCALL_64_after_hwframe+0x76/0x7e
  </TASK>

Let's add some checks that the attribute is properly sized and it's
the only one attribute inside the action.  Technically, there is no
real reason for OVS_KEY_ATTR_NSH to be there, as we know that we're
pushing an NSH header already, it just creates extra nesting, but
that's how uAPI works today.  So, keeping as it is.

Fixes: b2d0f5d5dc53 ("openvswitch: enable NSH support")
Reported-by: Junvy Yang <zhuque@tencent.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 net/openvswitch/flow_netlink.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 1cb4f97335d8..2d536901309e 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -2802,13 +2802,20 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
 	return err;
 }
 
-static bool validate_push_nsh(const struct nlattr *attr, bool log)
+static bool validate_push_nsh(const struct nlattr *a, bool log)
 {
+	struct nlattr *nsh_key = nla_data(a);
 	struct sw_flow_match match;
 	struct sw_flow_key key;
 
+	/* There must be one and only one NSH header. */
+	if (!nla_ok(nsh_key, nla_len(a)) ||
+	    nla_total_size(nla_len(nsh_key)) != nla_len(a) ||
+	    nla_type(nsh_key) != OVS_KEY_ATTR_NSH)
+		return false;
+
 	ovs_match_init(&match, &key, true, NULL);
-	return !nsh_key_put_from_nlattr(attr, &match, false, true, log);
+	return !nsh_key_put_from_nlattr(nsh_key, &match, false, true, log);
 }
 
 /* Return false if there are any non-masked bits set.
@@ -3389,7 +3396,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 					return -EINVAL;
 			}
 			mac_proto = MAC_PROTO_NONE;
-			if (!validate_push_nsh(nla_data(a), log))
+			if (!validate_push_nsh(a, log))
 				return -EINVAL;
 			break;
 
-- 
2.51.1

Re: [PATCH net] net: openvswitch: fix middle attribute validation in push_nsh() action

Posted by Aaron Conole 2 weeks ago

Ilya Maximets <i.maximets@ovn.org> writes:

> The push_nsh() action structure looks like this:
>
>  OVS_ACTION_ATTR_PUSH_NSH(OVS_KEY_ATTR_NSH(OVS_NSH_KEY_ATTR_BASE,...))
>
> The outermost OVS_ACTION_ATTR_PUSH_NSH attribute is OK'ed by the
> nla_for_each_nested() inside __ovs_nla_copy_actions().  The innermost
> OVS_NSH_KEY_ATTR_BASE/MD1/MD2 are OK'ed by the nla_for_each_nested()
> inside nsh_key_put_from_nlattr().  But nothing checks if the attribute
> in the middle is OK.  We don't even check that this attribute is the
> OVS_KEY_ATTR_NSH.  We just do a double unwrap with a pair of nla_data()
> calls - first time directly while calling validate_push_nsh() and the
> second time as part of the nla_for_each_nested() macro, which isn't
> safe, potentially causing invalid memory access if the size of this
> attribute is incorrect.  The failure may not be noticed during
> validation due to larger netlink buffer, but cause trouble later during
> action execution where the buffer is allocated exactly to the size:
>
>  BUG: KASAN: slab-out-of-bounds in nsh_hdr_from_nlattr+0x1dd/0x6a0 [openvswitch]
>  Read of size 184 at addr ffff88816459a634 by task a.out/22624
>
>  CPU: 8 UID: 0 PID: 22624 6.18.0-rc7+ #115 PREEMPT(voluntary)
>  Call Trace:
>   <TASK>
>   dump_stack_lvl+0x51/0x70
>   print_address_description.constprop.0+0x2c/0x390
>   kasan_report+0xdd/0x110
>   kasan_check_range+0x35/0x1b0
>   __asan_memcpy+0x20/0x60
>   nsh_hdr_from_nlattr+0x1dd/0x6a0 [openvswitch]
>   push_nsh+0x82/0x120 [openvswitch]
>   do_execute_actions+0x1405/0x2840 [openvswitch]
>   ovs_execute_actions+0xd5/0x3b0 [openvswitch]
>   ovs_packet_cmd_execute+0x949/0xdb0 [openvswitch]
>   genl_family_rcv_msg_doit+0x1d6/0x2b0
>   genl_family_rcv_msg+0x336/0x580
>   genl_rcv_msg+0x9f/0x130
>   netlink_rcv_skb+0x11f/0x370
>   genl_rcv+0x24/0x40
>   netlink_unicast+0x73e/0xaa0
>   netlink_sendmsg+0x744/0xbf0
>   __sys_sendto+0x3d6/0x450
>   do_syscall_64+0x79/0x2c0
>   entry_SYSCALL_64_after_hwframe+0x76/0x7e
>   </TASK>
>
> Let's add some checks that the attribute is properly sized and it's
> the only one attribute inside the action.  Technically, there is no
> real reason for OVS_KEY_ATTR_NSH to be there, as we know that we're
> pushing an NSH header already, it just creates extra nesting, but
> that's how uAPI works today.  So, keeping as it is.
>
> Fixes: b2d0f5d5dc53 ("openvswitch: enable NSH support")
> Reported-by: Junvy Yang <zhuque@tencent.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Checked it out using also the userspace kmod test suite, and the
kernel selftest scripts.  Change makes sense to me.

Reviewed-by: Aaron Conole <aconole@redhat.com>

Re: [PATCH net] net: openvswitch: fix middle attribute validation in push_nsh() action

Posted by Eelco Chaudron 2 weeks, 1 day ago


On 4 Dec 2025, at 11:53, Ilya Maximets wrote:

> The push_nsh() action structure looks like this:
>
>  OVS_ACTION_ATTR_PUSH_NSH(OVS_KEY_ATTR_NSH(OVS_NSH_KEY_ATTR_BASE,...))
>
> The outermost OVS_ACTION_ATTR_PUSH_NSH attribute is OK'ed by the
> nla_for_each_nested() inside __ovs_nla_copy_actions().  The innermost
> OVS_NSH_KEY_ATTR_BASE/MD1/MD2 are OK'ed by the nla_for_each_nested()
> inside nsh_key_put_from_nlattr().  But nothing checks if the attribute
> in the middle is OK.  We don't even check that this attribute is the
> OVS_KEY_ATTR_NSH.  We just do a double unwrap with a pair of nla_data()
> calls - first time directly while calling validate_push_nsh() and the
> second time as part of the nla_for_each_nested() macro, which isn't
> safe, potentially causing invalid memory access if the size of this
> attribute is incorrect.  The failure may not be noticed during
> validation due to larger netlink buffer, but cause trouble later during
> action execution where the buffer is allocated exactly to the size:
>
>  BUG: KASAN: slab-out-of-bounds in nsh_hdr_from_nlattr+0x1dd/0x6a0 [openvswitch]
>  Read of size 184 at addr ffff88816459a634 by task a.out/22624
>
>  CPU: 8 UID: 0 PID: 22624 6.18.0-rc7+ #115 PREEMPT(voluntary)
>  Call Trace:
>   <TASK>
>   dump_stack_lvl+0x51/0x70
>   print_address_description.constprop.0+0x2c/0x390
>   kasan_report+0xdd/0x110
>   kasan_check_range+0x35/0x1b0
>   __asan_memcpy+0x20/0x60
>   nsh_hdr_from_nlattr+0x1dd/0x6a0 [openvswitch]
>   push_nsh+0x82/0x120 [openvswitch]
>   do_execute_actions+0x1405/0x2840 [openvswitch]
>   ovs_execute_actions+0xd5/0x3b0 [openvswitch]
>   ovs_packet_cmd_execute+0x949/0xdb0 [openvswitch]
>   genl_family_rcv_msg_doit+0x1d6/0x2b0
>   genl_family_rcv_msg+0x336/0x580
>   genl_rcv_msg+0x9f/0x130
>   netlink_rcv_skb+0x11f/0x370
>   genl_rcv+0x24/0x40
>   netlink_unicast+0x73e/0xaa0
>   netlink_sendmsg+0x744/0xbf0
>   __sys_sendto+0x3d6/0x450
>   do_syscall_64+0x79/0x2c0
>   entry_SYSCALL_64_after_hwframe+0x76/0x7e
>   </TASK>
>
> Let's add some checks that the attribute is properly sized and it's
> the only one attribute inside the action.  Technically, there is no
> real reason for OVS_KEY_ATTR_NSH to be there, as we know that we're
> pushing an NSH header already, it just creates extra nesting, but
> that's how uAPI works today.  So, keeping as it is.
>
> Fixes: b2d0f5d5dc53 ("openvswitch: enable NSH support")
> Reported-by: Junvy Yang <zhuque@tencent.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Thanks, Ilya, for fixing this. One small nit about logging, but overall it looks good to me.

Acked-by: Eelco Chaudron echaudro@redhat.com

> ---
>  net/openvswitch/flow_netlink.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 1cb4f97335d8..2d536901309e 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -2802,13 +2802,20 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
>  	return err;
>  }
>
> -static bool validate_push_nsh(const struct nlattr *attr, bool log)
> +static bool validate_push_nsh(const struct nlattr *a, bool log)
>  {
> +	struct nlattr *nsh_key = nla_data(a);
>  	struct sw_flow_match match;
>  	struct sw_flow_key key;
>
> +	/* There must be one and only one NSH header. */
> +	if (!nla_ok(nsh_key, nla_len(a)) ||
> +	    nla_total_size(nla_len(nsh_key)) != nla_len(a) ||
> +	    nla_type(nsh_key) != OVS_KEY_ATTR_NSH)

Should we consider adding some logging based on the log flag here? Not a blocker, just noticed that nsh_key_put_from_nlattr() logs similar validation cases and wondered if we want the same consistency.

> +		return false;
> +
>  	ovs_match_init(&match, &key, true, NULL);
> -	return !nsh_key_put_from_nlattr(attr, &match, false, true, log);
> +	return !nsh_key_put_from_nlattr(nsh_key, &match, false, true, log);
>  }
>
>  /* Return false if there are any non-masked bits set.
> @@ -3389,7 +3396,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>  					return -EINVAL;
>  			}
>  			mac_proto = MAC_PROTO_NONE;
> -			if (!validate_push_nsh(nla_data(a), log))
> +			if (!validate_push_nsh(a, log))
>  				return -EINVAL;
>  			break;
>
> -- 
> 2.51.1

Re: [PATCH net] net: openvswitch: fix middle attribute validation in push_nsh() action

Posted by Ilya Maximets 2 weeks, 1 day ago

On 12/4/25 12:03 PM, Eelco Chaudron wrote:
> 
> 
> On 4 Dec 2025, at 11:53, Ilya Maximets wrote:
> 
>> The push_nsh() action structure looks like this:
>>
>>  OVS_ACTION_ATTR_PUSH_NSH(OVS_KEY_ATTR_NSH(OVS_NSH_KEY_ATTR_BASE,...))
>>
>> The outermost OVS_ACTION_ATTR_PUSH_NSH attribute is OK'ed by the
>> nla_for_each_nested() inside __ovs_nla_copy_actions().  The innermost
>> OVS_NSH_KEY_ATTR_BASE/MD1/MD2 are OK'ed by the nla_for_each_nested()
>> inside nsh_key_put_from_nlattr().  But nothing checks if the attribute
>> in the middle is OK.  We don't even check that this attribute is the
>> OVS_KEY_ATTR_NSH.  We just do a double unwrap with a pair of nla_data()
>> calls - first time directly while calling validate_push_nsh() and the
>> second time as part of the nla_for_each_nested() macro, which isn't
>> safe, potentially causing invalid memory access if the size of this
>> attribute is incorrect.  The failure may not be noticed during
>> validation due to larger netlink buffer, but cause trouble later during
>> action execution where the buffer is allocated exactly to the size:
>>
>>  BUG: KASAN: slab-out-of-bounds in nsh_hdr_from_nlattr+0x1dd/0x6a0 [openvswitch]
>>  Read of size 184 at addr ffff88816459a634 by task a.out/22624
>>
>>  CPU: 8 UID: 0 PID: 22624 6.18.0-rc7+ #115 PREEMPT(voluntary)
>>  Call Trace:
>>   <TASK>
>>   dump_stack_lvl+0x51/0x70
>>   print_address_description.constprop.0+0x2c/0x390
>>   kasan_report+0xdd/0x110
>>   kasan_check_range+0x35/0x1b0
>>   __asan_memcpy+0x20/0x60
>>   nsh_hdr_from_nlattr+0x1dd/0x6a0 [openvswitch]
>>   push_nsh+0x82/0x120 [openvswitch]
>>   do_execute_actions+0x1405/0x2840 [openvswitch]
>>   ovs_execute_actions+0xd5/0x3b0 [openvswitch]
>>   ovs_packet_cmd_execute+0x949/0xdb0 [openvswitch]
>>   genl_family_rcv_msg_doit+0x1d6/0x2b0
>>   genl_family_rcv_msg+0x336/0x580
>>   genl_rcv_msg+0x9f/0x130
>>   netlink_rcv_skb+0x11f/0x370
>>   genl_rcv+0x24/0x40
>>   netlink_unicast+0x73e/0xaa0
>>   netlink_sendmsg+0x744/0xbf0
>>   __sys_sendto+0x3d6/0x450
>>   do_syscall_64+0x79/0x2c0
>>   entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>   </TASK>
>>
>> Let's add some checks that the attribute is properly sized and it's
>> the only one attribute inside the action.  Technically, there is no
>> real reason for OVS_KEY_ATTR_NSH to be there, as we know that we're
>> pushing an NSH header already, it just creates extra nesting, but
>> that's how uAPI works today.  So, keeping as it is.
>>
>> Fixes: b2d0f5d5dc53 ("openvswitch: enable NSH support")
>> Reported-by: Junvy Yang <zhuque@tencent.com>
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> 
> Thanks, Ilya, for fixing this. One small nit about logging, but overall it looks good to me.
> 
> Acked-by: Eelco Chaudron echaudro@redhat.com
> 
>> ---
>>  net/openvswitch/flow_netlink.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>> index 1cb4f97335d8..2d536901309e 100644
>> --- a/net/openvswitch/flow_netlink.c
>> +++ b/net/openvswitch/flow_netlink.c
>> @@ -2802,13 +2802,20 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
>>  	return err;
>>  }
>>
>> -static bool validate_push_nsh(const struct nlattr *attr, bool log)
>> +static bool validate_push_nsh(const struct nlattr *a, bool log)
>>  {
>> +	struct nlattr *nsh_key = nla_data(a);
>>  	struct sw_flow_match match;
>>  	struct sw_flow_key key;
>>
>> +	/* There must be one and only one NSH header. */
>> +	if (!nla_ok(nsh_key, nla_len(a)) ||
>> +	    nla_total_size(nla_len(nsh_key)) != nla_len(a) ||
>> +	    nla_type(nsh_key) != OVS_KEY_ATTR_NSH)
> 
> Should we consider adding some logging based on the log flag here? Not a blocker,
> just noticed that nsh_key_put_from_nlattr() logs similar validation cases and
> wondered if we want the same consistency.

Our logging is not really consistent, we do not log in the same case for the
validate_set(), for example.  And I'm not sure if the log here would be useful
as it is very unlikely we can hit this condition without manually crafting the
attribute to be wrong.  We'll have a log later about garbage trailing data,
which should prompt a user to look at what they are sending down.

In general, we should convert all the logging here into extack, as logs are
very inconvenient and not specific enough in most cases.

But I can add something like this, if needed:

  OVS_NLERR(log, "push_nsh: Expected a single NSH header");

What do you think?

Best regards, Ilya Maximets.

Re: [PATCH net] net: openvswitch: fix middle attribute validation in push_nsh() action

Posted by Eelco Chaudron 2 weeks, 1 day ago


On 4 Dec 2025, at 12:36, Ilya Maximets wrote:

> On 12/4/25 12:03 PM, Eelco Chaudron wrote:
>>
>>
>> On 4 Dec 2025, at 11:53, Ilya Maximets wrote:
>>
>>> The push_nsh() action structure looks like this:
>>>
>>>  OVS_ACTION_ATTR_PUSH_NSH(OVS_KEY_ATTR_NSH(OVS_NSH_KEY_ATTR_BASE,...))
>>>
>>> The outermost OVS_ACTION_ATTR_PUSH_NSH attribute is OK'ed by the
>>> nla_for_each_nested() inside __ovs_nla_copy_actions().  The innermost
>>> OVS_NSH_KEY_ATTR_BASE/MD1/MD2 are OK'ed by the nla_for_each_nested()
>>> inside nsh_key_put_from_nlattr().  But nothing checks if the attribute
>>> in the middle is OK.  We don't even check that this attribute is the
>>> OVS_KEY_ATTR_NSH.  We just do a double unwrap with a pair of nla_data()
>>> calls - first time directly while calling validate_push_nsh() and the
>>> second time as part of the nla_for_each_nested() macro, which isn't
>>> safe, potentially causing invalid memory access if the size of this
>>> attribute is incorrect.  The failure may not be noticed during
>>> validation due to larger netlink buffer, but cause trouble later during
>>> action execution where the buffer is allocated exactly to the size:
>>>
>>>  BUG: KASAN: slab-out-of-bounds in nsh_hdr_from_nlattr+0x1dd/0x6a0 [openvswitch]
>>>  Read of size 184 at addr ffff88816459a634 by task a.out/22624
>>>
>>>  CPU: 8 UID: 0 PID: 22624 6.18.0-rc7+ #115 PREEMPT(voluntary)
>>>  Call Trace:
>>>   <TASK>
>>>   dump_stack_lvl+0x51/0x70
>>>   print_address_description.constprop.0+0x2c/0x390
>>>   kasan_report+0xdd/0x110
>>>   kasan_check_range+0x35/0x1b0
>>>   __asan_memcpy+0x20/0x60
>>>   nsh_hdr_from_nlattr+0x1dd/0x6a0 [openvswitch]
>>>   push_nsh+0x82/0x120 [openvswitch]
>>>   do_execute_actions+0x1405/0x2840 [openvswitch]
>>>   ovs_execute_actions+0xd5/0x3b0 [openvswitch]
>>>   ovs_packet_cmd_execute+0x949/0xdb0 [openvswitch]
>>>   genl_family_rcv_msg_doit+0x1d6/0x2b0
>>>   genl_family_rcv_msg+0x336/0x580
>>>   genl_rcv_msg+0x9f/0x130
>>>   netlink_rcv_skb+0x11f/0x370
>>>   genl_rcv+0x24/0x40
>>>   netlink_unicast+0x73e/0xaa0
>>>   netlink_sendmsg+0x744/0xbf0
>>>   __sys_sendto+0x3d6/0x450
>>>   do_syscall_64+0x79/0x2c0
>>>   entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>>   </TASK>
>>>
>>> Let's add some checks that the attribute is properly sized and it's
>>> the only one attribute inside the action.  Technically, there is no
>>> real reason for OVS_KEY_ATTR_NSH to be there, as we know that we're
>>> pushing an NSH header already, it just creates extra nesting, but
>>> that's how uAPI works today.  So, keeping as it is.
>>>
>>> Fixes: b2d0f5d5dc53 ("openvswitch: enable NSH support")
>>> Reported-by: Junvy Yang <zhuque@tencent.com>
>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
>>
>> Thanks, Ilya, for fixing this. One small nit about logging, but overall it looks good to me.
>>
>> Acked-by: Eelco Chaudron echaudro@redhat.com
>>
>>> ---
>>>  net/openvswitch/flow_netlink.c | 13 ++++++++++---
>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>>> index 1cb4f97335d8..2d536901309e 100644
>>> --- a/net/openvswitch/flow_netlink.c
>>> +++ b/net/openvswitch/flow_netlink.c
>>> @@ -2802,13 +2802,20 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
>>>  	return err;
>>>  }
>>>
>>> -static bool validate_push_nsh(const struct nlattr *attr, bool log)
>>> +static bool validate_push_nsh(const struct nlattr *a, bool log)
>>>  {
>>> +	struct nlattr *nsh_key = nla_data(a);
>>>  	struct sw_flow_match match;
>>>  	struct sw_flow_key key;
>>>
>>> +	/* There must be one and only one NSH header. */
>>> +	if (!nla_ok(nsh_key, nla_len(a)) ||
>>> +	    nla_total_size(nla_len(nsh_key)) != nla_len(a) ||
>>> +	    nla_type(nsh_key) != OVS_KEY_ATTR_NSH)
>>
>> Should we consider adding some logging based on the log flag here? Not a blocker,
>> just noticed that nsh_key_put_from_nlattr() logs similar validation cases and
>> wondered if we want the same consistency.
>
> Our logging is not really consistent, we do not log in the same case for the
> validate_set(), for example.  And I'm not sure if the log here would be useful
> as it is very unlikely we can hit this condition without manually crafting the
> attribute to be wrong.  We'll have a log later about garbage trailing data,
> which should prompt a user to look at what they are sending down.
>
> In general, we should convert all the logging here into extack, as logs are
> very inconvenient and not specific enough in most cases.
>
> But I can add something like this, if needed:
>
>   OVS_NLERR(log, "push_nsh: Expected a single NSH header");
>
> What do you think?

Reading your feedback, I’m fine with leaving it as is.

//Eelco

Re: [PATCH net] net: openvswitch: fix middle attribute validation in push_nsh() action

Posted by Ilya Maximets 2 weeks, 1 day ago

On 12/4/25 2:22 PM, Eelco Chaudron wrote:
> 
> 
> On 4 Dec 2025, at 12:36, Ilya Maximets wrote:
> 
>> On 12/4/25 12:03 PM, Eelco Chaudron wrote:
>>>
>>>
>>> On 4 Dec 2025, at 11:53, Ilya Maximets wrote:
>>>
>>>> The push_nsh() action structure looks like this:
>>>>
>>>>  OVS_ACTION_ATTR_PUSH_NSH(OVS_KEY_ATTR_NSH(OVS_NSH_KEY_ATTR_BASE,...))
>>>>
>>>> The outermost OVS_ACTION_ATTR_PUSH_NSH attribute is OK'ed by the
>>>> nla_for_each_nested() inside __ovs_nla_copy_actions().  The innermost
>>>> OVS_NSH_KEY_ATTR_BASE/MD1/MD2 are OK'ed by the nla_for_each_nested()
>>>> inside nsh_key_put_from_nlattr().  But nothing checks if the attribute
>>>> in the middle is OK.  We don't even check that this attribute is the
>>>> OVS_KEY_ATTR_NSH.  We just do a double unwrap with a pair of nla_data()
>>>> calls - first time directly while calling validate_push_nsh() and the
>>>> second time as part of the nla_for_each_nested() macro, which isn't
>>>> safe, potentially causing invalid memory access if the size of this
>>>> attribute is incorrect.  The failure may not be noticed during
>>>> validation due to larger netlink buffer, but cause trouble later during
>>>> action execution where the buffer is allocated exactly to the size:
>>>>
>>>>  BUG: KASAN: slab-out-of-bounds in nsh_hdr_from_nlattr+0x1dd/0x6a0 [openvswitch]
>>>>  Read of size 184 at addr ffff88816459a634 by task a.out/22624
>>>>
>>>>  CPU: 8 UID: 0 PID: 22624 6.18.0-rc7+ #115 PREEMPT(voluntary)
>>>>  Call Trace:
>>>>   <TASK>
>>>>   dump_stack_lvl+0x51/0x70
>>>>   print_address_description.constprop.0+0x2c/0x390
>>>>   kasan_report+0xdd/0x110
>>>>   kasan_check_range+0x35/0x1b0
>>>>   __asan_memcpy+0x20/0x60
>>>>   nsh_hdr_from_nlattr+0x1dd/0x6a0 [openvswitch]
>>>>   push_nsh+0x82/0x120 [openvswitch]
>>>>   do_execute_actions+0x1405/0x2840 [openvswitch]
>>>>   ovs_execute_actions+0xd5/0x3b0 [openvswitch]
>>>>   ovs_packet_cmd_execute+0x949/0xdb0 [openvswitch]
>>>>   genl_family_rcv_msg_doit+0x1d6/0x2b0
>>>>   genl_family_rcv_msg+0x336/0x580
>>>>   genl_rcv_msg+0x9f/0x130
>>>>   netlink_rcv_skb+0x11f/0x370
>>>>   genl_rcv+0x24/0x40
>>>>   netlink_unicast+0x73e/0xaa0
>>>>   netlink_sendmsg+0x744/0xbf0
>>>>   __sys_sendto+0x3d6/0x450
>>>>   do_syscall_64+0x79/0x2c0
>>>>   entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>>>   </TASK>
>>>>
>>>> Let's add some checks that the attribute is properly sized and it's
>>>> the only one attribute inside the action.  Technically, there is no
>>>> real reason for OVS_KEY_ATTR_NSH to be there, as we know that we're
>>>> pushing an NSH header already, it just creates extra nesting, but
>>>> that's how uAPI works today.  So, keeping as it is.
>>>>
>>>> Fixes: b2d0f5d5dc53 ("openvswitch: enable NSH support")
>>>> Reported-by: Junvy Yang <zhuque@tencent.com>
>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
>>>
>>> Thanks, Ilya, for fixing this. One small nit about logging, but overall it looks good to me.
>>>
>>> Acked-by: Eelco Chaudron echaudro@redhat.com
>>>
>>>> ---
>>>>  net/openvswitch/flow_netlink.c | 13 ++++++++++---
>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>>>> index 1cb4f97335d8..2d536901309e 100644
>>>> --- a/net/openvswitch/flow_netlink.c
>>>> +++ b/net/openvswitch/flow_netlink.c
>>>> @@ -2802,13 +2802,20 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
>>>>  	return err;
>>>>  }
>>>>
>>>> -static bool validate_push_nsh(const struct nlattr *attr, bool log)
>>>> +static bool validate_push_nsh(const struct nlattr *a, bool log)
>>>>  {
>>>> +	struct nlattr *nsh_key = nla_data(a);
>>>>  	struct sw_flow_match match;
>>>>  	struct sw_flow_key key;
>>>>
>>>> +	/* There must be one and only one NSH header. */
>>>> +	if (!nla_ok(nsh_key, nla_len(a)) ||
>>>> +	    nla_total_size(nla_len(nsh_key)) != nla_len(a) ||
>>>> +	    nla_type(nsh_key) != OVS_KEY_ATTR_NSH)
>>>
>>> Should we consider adding some logging based on the log flag here? Not a blocker,
>>> just noticed that nsh_key_put_from_nlattr() logs similar validation cases and
>>> wondered if we want the same consistency.
>>
>> Our logging is not really consistent, we do not log in the same case for the
>> validate_set(), for example.  And I'm not sure if the log here would be useful
>> as it is very unlikely we can hit this condition without manually crafting the
>> attribute to be wrong.  We'll have a log later about garbage trailing data,
>> which should prompt a user to look at what they are sending down.
>>
>> In general, we should convert all the logging here into extack, as logs are
>> very inconvenient and not specific enough in most cases.
>>
>> But I can add something like this, if needed:
>>
>>   OVS_NLERR(log, "push_nsh: Expected a single NSH header");
>>
>> What do you think?
> 
> Reading your feedback, I’m fine with leaving it as is.

OK.  I will not send a v2 for this then, unless there will be some other feedback.

Best regards, Ilya Maximets.