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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 30 May 2024 10:26:27 +0800
From: Jason Wang <jasowang@...hat.com>
To: Si-Wei Liu <si-wei.liu@...cle.com>
Cc: willemdebruijn.kernel@...il.com, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, bpf@...r.kernel.org, mst@...hat.com, 
	boris.ostrovsky@...cle.com
Subject: Re: [PATCH] net: tap: validate metadata and length for XDP buff
 before building up skb

On Thu, May 30, 2024 at 8:54 AM Si-Wei Liu <si-wei.liu@...cle.com> 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@...hat.com
> Signed-off-by: Si-Wei Liu <si-wei.liu@...cle.com>
> ---
>  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
>