On 7/25/26 6:31 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 calling check_header() in key_extract() before accessing the
> Ethernet header.
>
> Fixes: 217ac77a3c25 ("openvswitch: allow L3 netdev ports")
> Reported-by: [email protected]
> Signed-off-by: Cen Zhang (Microsoft) <[email protected]>
> ---
> v4: Update documentation and move variables into the Ethernet block.
> v3: Use check_header() per review, fix format issue.
> v2: Moved the check into key_extract() per Ilya Maximets.
> Link: https://lore.kernel.org/all/[email protected]
>
> net/openvswitch/flow.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 66366982f604..8ad0be37a65a 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -889,8 +889,6 @@ static int key_extract_l3l4(struct sk_buff *skb, struct
> sw_flow_key *key)
> * Ethernet header
> * @key: output flow key
> *
> - * The caller must ensure that skb->len >= ETH_HLEN.
> - *
> * Initializes @skb header fields as follows:
> *
> * - skb->mac_header: the L2 header.
> @@ -910,8 +908,6 @@ static int key_extract_l3l4(struct sk_buff *skb, struct
> sw_flow_key *key)
> */
> static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> {
> - struct ethhdr *eth;
> -
> /* Flags are always used as part of stats */
> key->tp.flags = 0;
>
> @@ -926,6 +922,12 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> skb_reset_network_header(skb);
> key->eth.type = skb->protocol;
> } else {
> + struct ethhdr *eth;
> + int err = check_header(skb, ETH_HLEN);
Please, use the reverse x-mass tree ordering for variable declaration,
i.e. lines longest to shortest. In this particular case, just move
the initialization to a separate line:
struct ethhdr *eth;
int err;
err = check_header(skb, ETH_HLEN);
if (unlikely(err)) ...
Otherwise, LGTM.
Sashiko reports two pre-existing issues that are not related to this patch:
1. Potential use of uninitialized bits of the key during lookup if the
code takes the positive branch of the if statement, since the addresses
remain uninitialized in the flow key structure.
There should be no functional issues, as those bits will be bitwise
ANDed with zeroes form the mask before the lookup in most cases, and
in case of L3 key comparison with the L2 flow where the addresses
will not be zero, the key must not match anyway due to prior mismatch
on the eth protocol. However, it is possible that KASAN will not be
happy with these operations producing warnings. I'll try to take a
closer look at this one. The zeroing out of the key was previously
removed for performance reasons, since it doesn't affect flow matching.
But having KASAN splats is also not great and potentially dangerous.
2. There is an unlikely case where ovs_flow_key_update() might fail,
which would cause skb leak on failure of recirc() or ct() actions.
I'm not really sure if this condition is reachable in practice as it
requires parsing to fail on a packet that we previously successfully
parsed and then modified. But should be fixed nevertheless.
I'll send a separate small patch set for this.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
