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

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

[Ren Wei]

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",
-- 
2.43.0

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

[Ilya Maximets]

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.

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.

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

[Ilya Maximets]

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.