On 4/25/26 10:19 PM, Ilya Maximets wrote: > On 4/25/26 6:40 PM, Ren Wei wrote: >> From: Douya Le <[email protected]> >> >> The NSH header length is encoded in 4-byte words in a 6-bit field. >> The current push_nsh validation only checks the MD2 payload against >> NSH_CTX_HDRS_MAX_LEN, which still allows metadata sizes that cannot be >> represented once the base header is included. >> >> Reject MD2 metadata lengths whose total NSH header size cannot be >> encoded exactly in the NSH length field, whether because the field >> would wrap or because the length is not a multiple of 4 bytes. >> >> Fixes: b2d0f5d5dc53 ("openvswitch: enable NSH support") >> Cc: [email protected] >> Reported-by: Yuan Tan <[email protected]> >> Reported-by: Yifan Wu <[email protected]> >> Reported-by: Juefei Pu <[email protected]> >> Reported-by: Xin Liu <[email protected]> >> Tested-by: Douya Le <[email protected]> >> Signed-off-by: Douya Le <[email protected]> >> Signed-off-by: Ren Wei <[email protected]> >> --- >> net/openvswitch/flow_netlink.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c >> index 13052408a132..8a1ae5309c2c 100644 >> --- a/net/openvswitch/flow_netlink.c >> +++ b/net/openvswitch/flow_netlink.c >> @@ -1432,7 +1432,13 @@ static int nsh_key_put_from_nlattr(const struct >> nlattr *attr, >> >> has_md2 = true; >> mdlen = nla_len(a); >> - if (mdlen > NSH_CTX_HDRS_MAX_LEN || mdlen <= 0) { >> + /* The NSH length field stores the total header size >> + * in 4-byte words in 6 bits. Reject MD2 metadata >> + * lengths that cannot be encoded exactly or would >> + * make the length field wrap. >> + */ >> + if (mdlen <= 0 || !IS_ALIGNED(mdlen, 4) || >> + NSH_BASE_HDR_LEN + mdlen > (NSH_LEN_MASK << 2)) { >> OVS_NLERR( >> log, >> "Invalid MD length %d for MD type %d", > > I don't think the check is a problem here, the problem is that the constants > are not correct. We actually had them fixed in userspace, but looks like > no-one ported the change to the kernel side: > > https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/ > > Adjusting the macros should solve the potential overflow issue. However, > even if the value overflows the 6-bit length, it will just be truncated to > a smaller value and nothing bad should happen in this case, as far as header > push is concerned. The context was wrong already, so pushing a truncated > context doesn't make it more wrong. But we should still fix the constants, > so the checks make sense, of course.
I posted a fix for the macros here: https://lore.kernel.org/netdev/[email protected]/T/#u > > The same for alignment, if the user provides an invalid header that is not > multiple of 4, the code will just truncate it to the previous multiple of 4, > which should not be a problem, as it wasn't correct in the first place. We > could add this check just to warn the user that they are doing something > wrong, > but not having it should not cause any real issues. > > Note, the userpace change linked above did fix the real issue in userspace, > but for the kernel side there seem to be no real memory related issues caused > by the wrong constants. > > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
