Message-ID: <2025060449-arena-exceeding-a090@gregkh>
Date: Wed, 4 Jun 2025 10:03:29 +0200
From: Greg KH <greg@...ah.com>
To: Ilya Maximets <i.maximets@....org>
Cc: 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 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?

confused,

greg k-h

Powered by blists - more mailing lists