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

[Jason Wang]

On Fri, May 31, 2024 at 5:05 AM Si-Wei Liu <si-wei.liu@xxxxxxxxxx> wrote:
>
>
>
> On 5/29/2024 7:26 PM, Jason Wang wrote:
> > 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?
> Not sure. Initially I thought about this as well, but changed mind.
> Although the TUN_MSG_PTR interface was specifically customized for
> vhost-net in the kernel, could there be any userspace app do sendmsg()
> also with customized TUN_MSG_PTR control message over tap's fd? If
> that's possible in reality, I guess limiting the fix to only vhost-net
> in the kernel is narrow scoped.

I think not, sendmsg() can't be used for tuntap.

>
> Additionally, it seems just the skb delivery path in the tap driver (or
> tuntap) that populates the relevant skb field needs the ETH_HLEN check,
> the XDP fast path can just transmit or forward xdp buff as-is without
> having to check the (header) length of payload data. That said, it may
> break some guest applications that intentionally send out short frames
> (for test purpose?)

I don't think so, various hypervisors will just drop short ethernet frames.

> if unconditionally drop all of them from the vhost-net.
>
> > It seems tuntap suffers from this as well.
> True, theoretically I can fix tuntap as well, but I don't have a setup
> to test out the code change thoroughly. Any volunteer here to do so
> (test it or fix it)?

It should be easier than the tap. If you can't find one, I can test.

>
> >
> > 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.
> Hmmm, WARN_ON may be fine (I don't see userspace is prevented from
> fabricating such invalid addresses through the TUN_MSG_PTR uAPI).

Tap doesn't export socket objects to userspace, so it's not an uAPI.

Thanks

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