Message-ID: <7bc258ad-3f65-4d6e-a9f5-840a6c174d90@ovn.org>
Date: Wed, 4 Jun 2025 10:19:45 +0200
From: Ilya Maximets <i.maximets@....org>
To: Greg KH <greg@...ah.com>
Cc: i.maximets@....org, Sasha Levin <sashal@...nel.org>,
 patches@...ts.linux.dev, stable@...r.kernel.org,
 Eelco Chaudron <echaudro@...hat.com>, Simon Horman <horms@...nel.org>,
 Jakub Kicinski <kuba@...nel.org>, aconole@...hat.com,
 netdev@...r.kernel.org, dev@...nvswitch.org
Subject: Re: [PATCH AUTOSEL 6.15 044/118] openvswitch: Stricter validation for
 the userspace action

On 6/4/25 10:03 AM, Greg KH wrote:
> On Wed, Jun 04, 2025 at 09:57:20AM +0200, Ilya Maximets wrote:
>> On 6/4/25 2:49 AM, Sasha Levin wrote:
>>> From: Eelco Chaudron <echaudro@...hat.com>
>>>
>>> [ Upstream commit 88906f55954131ed2d3974e044b7fb48129b86ae ]
>>>
>>> This change enhances the robustness of validate_userspace() by ensuring
>>> that all Netlink attributes are fully contained within the parent
>>> attribute. The previous use of nla_parse_nested_deprecated() could
>>> silently skip trailing or malformed attributes, as it stops parsing at
>>> the first invalid entry.
>>>
>>> By switching to nla_parse_deprecated_strict(), we make sure only fully
>>> validated attributes are copied for later use.
>>>
>>> Signed-off-by: Eelco Chaudron <echaudro@...hat.com>
>>> Reviewed-by: Simon Horman <horms@...nel.org>
>>> Acked-by: Ilya Maximets <i.maximets@....org>
>>> Link: https://patch.msgid.link/67eb414e2d250e8408bb8afeb982deca2ff2b10b.1747037304.git.echaudro@redhat.com
>>> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
>>> Signed-off-by: Sasha Levin <sashal@...nel.org>
>>> ---
>>>
>>> **YES** This commit should be backported to stable kernel trees. ##
>>> Analysis **Commit Overview:** The commit changes `validate_userspace()`
>>> function in `net/openvswitch/flow_netlink.c` by replacing
>>> `nla_parse_nested_deprecated()` with `nla_parse_deprecated_strict()` to
>>> ensure stricter validation of Netlink attributes for the userspace
>>> action. **Specific Code Changes:** The key change is on lines 3052-3054:
>>> ```c // Before: error = nla_parse_nested_deprecated(a,
>>> OVS_USERSPACE_ATTR_MAX, attr, userspace_policy, NULL); // After: error =
>>> nla_parse_deprecated_strict(a, OVS_USERSPACE_ATTR_MAX, nla_data(attr),
>>> nla_len(attr), userspace_policy, NULL); ``` **Why This Should Be
>>> Backported:** 1. **Security Enhancement:** This commit addresses a
>>> parsing vulnerability where malformed attributes could be silently
>>> ignored. The original `nla_parse_nested_deprecated()` stops parsing at
>>> the first invalid entry, potentially allowing trailing malformed data to
>>> bypass validation. 2. **Robustness Fix:** The change ensures all netlink
>>> attributes are fully contained within the parent attribute bounds,
>>> preventing potential buffer over-reads or under-reads that could lead to
>>> security issues. 3. **Pattern Consistency:** Looking at the git blame
>>> output (lines 3085-3087), we can see that
>>> `nla_parse_deprecated_strict()` was already introduced in 2019 by commit
>>> 8cb081746c031 and is used elsewhere in the same file for similar
>>> validation (e.g., `validate_and_copy_check_pkt_len()` function). 4.
>>> **Low Risk:** This is a small, contained change that only affects input
>>> validation - it doesn't change functionality or introduce new features.
>>> The change is defensive and follows existing patterns in the codebase.
>>> 5. **Similar Precedent:** This commit is very similar to the validated
>>> "Similar Commit #2" which was marked for backporting (status: YES). That
>>> commit also dealt with netlink attribute validation safety in
>>> openvswitch (`validate_set()` function) and was considered suitable for
>>> stable trees. 6. **Critical Subsystem:** Open vSwitch is a critical
>>> networking component used in virtualization and container environments.
>>> Input validation issues in this subsystem could potentially be exploited
>>> for privilege escalation or denial of service. 7. **Clear Intent:** The
>>> commit message explicitly states this "enhances robustness" and ensures
>>> "only fully validated attributes are copied for later use," indicating
>>> this is a defensive security improvement. **Risk Assessment:** - Very
>>> low regression risk - No API changes - Only affects error handling paths
>>> - Follows established validation patterns in the same codebase This
>>> commit fits perfectly into the stable tree criteria: it's an important
>>> security/robustness fix, has minimal risk of regression, is well-
>>> contained, and addresses a clear validation vulnerability in a critical
>>> kernel subsystem.
>>
>> This change is one of two patches created for userspace action.  With an
>> intentional split - one for net and one for net-next  First one was the
>> actual fix that addressed a real bug:
>>   6beb6835c1fb ("openvswitch: Fix unsafe attribute parsing in output_userspace()")
>>   https://lore.kernel.org/netdev/0bd65949df61591d9171c0dc13e42cea8941da10.1746541734.git.echaudro@redhat.com/
>>
>> This second change (this patch) was intended for -next only as it doesn't
>> fix any real issue, but affects uAPI, and so should NOT be backported.
> 
> Why would you break the user api in a newer kernel?  That feels wrong,
> as any change should be able to be backported without any problems.
> 
> If this is a userspace break, why isn't it reverted?

It doesn't break existing userspace that we know of.  However, it does make
the parsing of messages from userspace a bit more strict, and some messages
that would've worked fine before (e.g. having extra unrecognized attributes)
will no longer work.  There is no reason for userspace to ever rely on such
behavior, but AFAICT, historically, different parts of kernel networking
(e.g. tc-flower) introduced similar changes (making netlink stricter) on
net-next without backporting them.  Maybe Jakub can comment on that.

All in all, I do not expect any existing applications to break, but it seems
a little strange to touch uAPI in stable trees.

Best regards, Ilya Maximets.

Powered by blists - more mailing lists