Message-ID: <38ef1815-5bc1-4391-b487-05a18e84c94e@ovn.org>
Date: Wed, 4 Jun 2025 09:57:20 +0200
From: Ilya Maximets <i.maximets@....org>
To: Sasha Levin <sashal@...nel.org>, patches@...ts.linux.dev,
 stable@...r.kernel.org
Cc: i.maximets@....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 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.

Best regards, Ilya Maximets.

> 
>  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 518be23e48ea9..ad64bb9ab5e25 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;

Powered by blists - more mailing lists