netdev - Re: [PATCH net-next] openvswitch: Fix userspace attribute length validation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <12cd8c5b-d399-4cc5-8b20-d80d567ba0cb@ovn.org>
Date: Fri, 9 May 2025 01:07:38 +0200
From: Ilya Maximets <i.maximets@....org>
To: Eelco Chaudron <echaudro@...hat.com>, netdev@...r.kernel.org
Cc: i.maximets@....org, dev@...nvswitch.org, aconole@...hat.com,
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, horms@...nel.org
Subject: Re: [PATCH net-next] openvswitch: Fix userspace attribute length
 validation

On 5/8/25 11:32 AM, Eelco Chaudron wrote:
> The current implementation of validate_userspace() does not verify
> whether all Netlink attributes fit within the parent attribute. This
> is because nla_parse_nested_deprecated() stops processing Netlink
> attributes as soon as an invalid one is encountered.
> 
> To address this, we use nla_parse_deprecated_strict() which will
> return an error upon encountering attributes with an invalid size.
> 
> Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
> Signed-off-by: Eelco Chaudron <echaudro@...hat.com>
> ---

Hi, Eelco.  Thanks for the patch!

The code seems fine at a glance, though I didn't test it yet.

But I have a few comments about the commit message.

This change is not a bug fix - accepting unknown attributes or
ignoring malformed ones is just a design choice.  So, IMO, the
patch subject and the commit message should not be worded as if
we're fixing some bug here.  And it should also not include the
'Fixes' tag as this change should go to net-next only and must
not be backported.

So, I'd suggest to re-word the subject line so it doesn't contain
the word 'Fix', e.g.:
  net: openvswitch: stricter validation for the userspace action

And re-wording the commit message explaining why it is better
to have strict validation without implying that this change is
fixing some bugs.  This includes removing the 'Fixes' tag.

Best regards, Ilya Maximets.

>  net/openvswitch/flow_netlink.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 518be23e48ea..ad64bb9ab5e2 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -3049,7 +3049,8 @@ static int validate_userspace(const struct nlattr *attr)
>  	struct nlattr *a[OVS_USERSPACE_ATTR_MAX + 1];
>  	int error;
>  
> -	error = nla_parse_nested_deprecated(a, OVS_USERSPACE_ATTR_MAX, attr,
> +	error = nla_parse_deprecated_strict(a, OVS_USERSPACE_ATTR_MAX,
> +					    nla_data(attr), nla_len(attr),
>  					    userspace_policy, NULL);
>  	if (error)
>  		return error;