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

[Greg KH <greg@kroah.com>]

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.

thanks,

greg k-h