Re: [PATCH] net: tap: validate metadata and length for XDP buff before building up skb

[Jason Wang]

On Thu, May 30, 2024 at 8:54 AM Si-Wei Liu <si-wei.liu@xxxxxxxxxx> wrote:
>
> The cited commit missed to check against the validity of the length
> and various pointers on the XDP buff metadata in the tap_get_user_xdp()
> path, which could cause a corrupted skb to be sent downstack. For
> instance, tap_get_user() prohibits short frame which has the length
> less than Ethernet header size from being transmitted, while the
> skb_set_network_header() in tap_get_user_xdp() would set skb's
> network_header regardless of the actual XDP buff data size. This
> could either cause out-of-bound access beyond the actual length, or
> confuse the underlayer with incorrect or inconsistent header length
> in the skb metadata.
>
> Propose to drop any frame shorter than the Ethernet header size just
> like how tap_get_user() does. While at it, validate the pointers in
> XDP buff to avoid potential size overrun.
>
> Fixes: 0efac27791ee ("tap: accept an array of XDP buffs through sendmsg()")
> Cc: jasowang@xxxxxxxxxx
> Signed-off-by: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
> ---
>  drivers/net/tap.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index bfdd3875fe86..69596479536f 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -1177,6 +1177,13 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
>         struct sk_buff *skb;
>         int err, depth;
>
> +       if (unlikely(xdp->data < xdp->data_hard_start ||
> +                    xdp->data_end < xdp->data ||
> +                    xdp->data_end - xdp->data < ETH_HLEN)) {
> +               err = -EINVAL;
> +               goto err;
> +       }

For ETH_HLEN check, is it better to do it in vhost-net? It seems
tuntap suffers from this as well.

And for the check for other xdp fields, it deserves a BUG_ON() or at
least WARN_ON() as they are set by vhost-net.

Thanks

> +
>         if (q->flags & IFF_VNET_HDR)
>                 vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz);
>
> --
> 2.39.3
>