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] [day] [month] [year] [list]

Message-ID: <36984A9D-1811-4F33-A6C4-310E44613E2E@redhat.com>
Date: Mon, 12 May 2025 10:10:19 +0200
From: Eelco Chaudron <echaudro@...hat.com>
To: Ilya Maximets <i.maximets@....org>
Cc: netdev@...r.kernel.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 9 May 2025, at 1:07, Ilya Maximets wrote:

> 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.

Thanks for the feedback Ilya! As I got no other reviews/feedback, I’ve sent a v2 addressing your feedback.

Cheers,

Eelco

>>  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;