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... 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:
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.1746...
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