Re: [PATCH v2] net/openvswitch: check Ethernet header length before key_extract()

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

On 7/23/26 6:23 AM, Cen Zhang (Microsoft) wrote:
> When a packet arrives on an ARPHRD_NONE device (e.g. TUN),
> ovs_flow_key_extract() trusts the user-provided skb->protocol field: if
> it is ETH_P_TEB, the packet is classified as MAC_PROTO_ETHERNET and
> key_extract() is called without ensuring the skb has ETH_HLEN (14) bytes
> of linear data. key_extract() unconditionally pulls 2 * ETH_ALEN bytes
> for MAC addresses and parse_ethertype() pulls 2 more, either of which
> triggers a kernel BUG in __skb_pull() when the linear area is too small.
> 
>   kernel BUG at include/linux/skbuff.h:2848!
>   RIP: 0010:key_extract+0xa7e/0xd90 net/openvswitch/flow.c:933
>   ovs_flow_key_extract+0x419/0xa70
>   ovs_vport_receive+0x222/0x390
>   netdev_frame_hook+0x3e0/0x630
>   tun_get_user+0x2d0c/0x38e0
> 
> Fixed by adding checks in key_extract() before pulling the Ethernet
> header.
> 
> Fixes: 217ac77a3c25 ("openvswitch: allow L3 netdev ports")
> Reported-by: AutonomousCodeSecurity@microsoft.com
> Link: https://lore.kernel.org/all/20260721143602.64677-1-blbllhy@gmail.com

This link should not be here, it doesn't make sense as part of the commit
message.  It should be in the change log under the cut line.
Maintainers will add the Link tag for the version that will actually be
applied.

Adding the Link tag to workaround the checkpatch warning also doesn't make
sense.  The warning is there for a reason.  If there is a separate public
report, then you can link it, otherwise just live with the warning or drop
the Reported-by tag.

Also, the tree name 'net' is missing in the subject prefix.

> Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
> ---
> v2: Moved the check into key_extract() per Ilya Maximets.

You also changed the check, which is not right, IMO.  More on that below.

> 
>  net/openvswitch/flow.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 66366982f604..de7b77d6702a 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -926,6 +926,9 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>  		skb_reset_network_header(skb);
>  		key->eth.type = skb->protocol;
>  	} else {
> +		if (unlikely(!pskb_may_pull(skb, ETH_HLEN)))
> +			return -EINVAL;
> +

We should keep the check_header as it was in the previous version of
the patch.  It distinguishes the cases of the length mismatch and the
memory allocation failure.  While it's not critical in this scenario,
it is more consistent with the rest of the code.  The error can be
sent to userspace from this location and it should be more granular
than just EINVAL in all cases, and the check_header() provides this
granularity.

Best regards, Ilya Maximets.