Re: [PATCH AUTOSEL 6.15 044/118] openvswitch: Stricter validation for the userspace action

[Ilya Maximets <i.maximets@ovn.org>]

On 6/4/25 10:28 AM, Greg KH wrote:
> On Wed, Jun 04, 2025 at 10:19:45AM +0200, Ilya Maximets wrote:
>> 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@redhat.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@redhat.com>
>>>>> Reviewed-by: Simon Horman <horms@kernel.org>
>>>>> Acked-by: Ilya Maximets <i.maximets@ovn.org>
>>>>> Link: https://patch.msgid.link/67eb414e2d250e8408bb8afeb982deca2ff2b10b.1747037304.git.echaudro@redhat.com
>>>>> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>>>>> Signed-off-by: Sasha Levin <sashal@kernel.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.
> 
> Nothing that ends up on Linus's tree should not be allowed also to be in
> a stable kernel release as there is no difference in the "rule" that "we
> will not break userspace".
> 
> So this isn't an issue here, if you need/want to make parsing more
> strict, due to bugs or whatever, then great, let's make it more strict
> as long as it doesn't break anyone's current system.  It doesn't matter
> if this is in Linus's release or in a stable release, same rule holds
> for both.

Makes total sense, thanks.  No objections from my side then.

Best regards, Ilya Maximets.