Received-SPF: pass (google.com: domain of linux-kernel+bounces-196076-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <1717026141-25716-1-git-send-email-si-wei.liu@oracle.com>
 <CACGkMEugdcKjxMA_3+-gfh4wKOP5vTvYOb2V+MP7VxDiZ6EhiA@mail.gmail.com> <490d42c8-5361-4db4-a5d1-3f992f4b8003@oracle.com>
In-Reply-To: <490d42c8-5361-4db4-a5d1-3f992f4b8003@oracle.com>
From: Jason Wang <jasowang@redhat.com>
Date: Fri, 31 May 2024 08:26:17 +0800
Message-ID: <CACGkMEvdyFxL1dW81H+=uEqZWqH5-AVOE_0y3DCO-pK4ap==fg@mail.gmail.com>
Subject: Re: [PATCH] net: tap: validate metadata and length for XDP buff
 before building up skb
To: Si-Wei Liu <si-wei.liu@oracle.com>
Cc: willemdebruijn.kernel@gmail.com, davem@davemloft.net, edumazet@google.com, 
	kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org, 
	linux-kernel@vger.kernel.org, bpf@vger.kernel.org, mst@redhat.com, 
	boris.ostrovsky@oracle.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, May 31, 2024 at 5:05=E2=80=AFAM Si-Wei Liu <si-wei.liu@oracle.com> =
wrote:
>
>
>
> On 5/29/2024 7:26 PM, Jason Wang wrote:
> > On Thu, May 30, 2024 at 8:54=E2=80=AFAM Si-Wei Liu <si-wei.liu@oracle.c=
om> 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 sendms=
g()")
> >> Cc: jasowang@redhat.com
> >> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.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 =3D -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 =3D READ_ONCE(q->vnet_hdr_sz);
> >>
> >> --
> >> 2.39.3
> >>
>