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 <[email protected]>
> Signed-off-by: Ilya Maximets <[email protected]>
Thanks, Ilya, for fixing this. One small nit about logging, but overall it
looks good to me.
Acked-by: Eelco Chaudron [email protected]
> ---
> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
