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 <echau...@redhat.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;
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
