Re: [PATCH net 1/1] openvswitch: reject oversized NSH MD2 metadata in push_nsh

[Ilya Maximets <i.maximets@ovn.org>]

On 4/25/26 10:19 PM, Ilya Maximets wrote:
> On 4/25/26 6:40 PM, Ren Wei wrote:
>> From: Douya Le <ldy3087146292@gmail.com>
>>
>> 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: stable@kernel.org
>> Reported-by: Yuan Tan <yuantan098@gmail.com>
>> Reported-by: Yifan Wu <yifanwucs@gmail.com>
>> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
>> Reported-by: Xin Liu <bird@lzu.edu.cn>
>> Tested-by: Douya Le <ldy3087146292@gmail.com>
>> Signed-off-by: Douya Le <ldy3087146292@gmail.com>
>> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
>> ---
>>  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/1540503710-23597-1-git-send-email-pkusunyifeng@gmail.com/
> 
> 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/20260507120434.2962505-1-i.maximets@ovn.org/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.