| 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
| ||
|
Date: Tue, 22 Aug 2023 19:48:05 +0200
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: "Sarkar, Tirthendu" <tirthendu.sarkar@...el.com>
CC: Stanislav Fomichev <sdf@...gle.com>, "bpf@...r.kernel.org"
<bpf@...r.kernel.org>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"bjorn@...nel.org" <bjorn@...nel.org>, "Karlsson, Magnus"
<magnus.karlsson@...el.com>, "jonathan.lemon@...il.com"
<jonathan.lemon@...il.com>, "davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
"ast@...nel.org" <ast@...nel.org>, "daniel@...earbox.net"
<daniel@...earbox.net>, "martin.lau@...ux.dev" <martin.lau@...ux.dev>,
"dan.carpenter@...aro.org" <dan.carpenter@...aro.org>
Subject: Re: [PATCH bpf-next v2] xsk: fix xsk_build_skb() error: 'skb'
dereferencing possible ERR_PTR()
On Wed, Aug 16, 2023 at 08:04:52AM +0200, Sarkar, Tirthendu wrote:
> > -----Original Message-----
> > From: Stanislav Fomichev <sdf@...gle.com>
> > Sent: Tuesday, August 15, 2023 11:51 PM
> > On 08/15, Tirthendu Sarkar wrote:
> > > xsk_build_skb_zerocopy() may return an error other than -EAGAIN and
> > this
> > > is received as skb and used later in xsk_set_destructor_arg() and
> > > xsk_drop_skb() which must operate on a valid skb.
> > >
> > > Set -EOVERFLOW as error when MAX_SKB_FRAGS are exceeded and
> > packet needs
> > > to be dropped and use this to distinguish against all other error cases
> > > where allocation needs to be retried.
Please be explicit - say that you're changing this error code
in xsk_build_skb_zerocopy() otherwise it's not clear.
You're not saying anything about kfree_skb() that is added when
skb_store_bits() failed. This code is non-trivial so all of the changes
need to be described.
Also did we test this patch? I believe it would require us to run xdpsock
against virtio net device?
> > >
> > > Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@...el.com>
> > > Reported-by: kernel test robot <lkp@...el.com>
> > > Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> > > Closes: https://lore.kernel.org/r/202307210434.OjgqFcbB-lkp@intel.com/
> > > Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx
> > path")
> > >
> > > Changelog:
> > > v1 -> v2:
> > > - Removed err as a parameter to xsk_build_skb_zerocopy()
> > > [Stanislav Fomichev]
> > > - use explicit error to distinguish packet drop vs retry
> > > ---
> > > net/xdp/xsk.c | 22 +++++++++++++---------
> > > 1 file changed, 13 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > index fcfc8472f73d..55f8b9b0e06d 100644
> > > --- a/net/xdp/xsk.c
> > > +++ b/net/xdp/xsk.c
> > > @@ -602,7 +602,7 @@ static struct sk_buff
> > *xsk_build_skb_zerocopy(struct xdp_sock *xs,
> > >
> > > for (copied = 0, i = skb_shinfo(skb)->nr_frags; copied < len; i++) {
> > > if (unlikely(i >= MAX_SKB_FRAGS))
> > > - return ERR_PTR(-EFAULT);
> > > + return ERR_PTR(-EOVERFLOW);
> > >
> > > page = pool->umem->pgs[addr >> PAGE_SHIFT];
> > > get_page(page);
> > > @@ -655,15 +655,17 @@ static struct sk_buff *xsk_build_skb(struct
> > xdp_sock *xs,
> > > skb_put(skb, len);
> > >
> > > err = skb_store_bits(skb, 0, buffer, len);
> > > - if (unlikely(err))
> > > + if (unlikely(err)) {
> > > + kfree_skb(skb);
> > > goto free_err;
> > > + }
> > > } else {
> > > int nr_frags = skb_shinfo(skb)->nr_frags;
> > > struct page *page;
> > > u8 *vaddr;
> > >
> > > if (unlikely(nr_frags == (MAX_SKB_FRAGS - 1) &&
> > xp_mb_desc(desc))) {
> > > - err = -EFAULT;
> > > + err = -EOVERFLOW;
> > > goto free_err;
> > > }
> > >
> > > @@ -690,12 +692,14 @@ static struct sk_buff *xsk_build_skb(struct
> > xdp_sock *xs,
> > > return skb;
> > >
> > > free_err:
> > > - if (err == -EAGAIN) {
> > > - xsk_cq_cancel_locked(xs, 1);
> > > - } else {
> > > - xsk_set_destructor_arg(skb);
> > > - xsk_drop_skb(skb);
> > > + if (err == -EOVERFLOW) {
> >
> > Don't think this will work? We have some other error paths in xsk_build_skb
> > that are not -EOVERFLOW that still need kfree_skb, right?
> >
>
> There are 4 possible error paths in xsk_build_skb():
> 1. sock_alloc_send_skb: skb is NULL; retry
> 2. skb_store_bits : free skb and retry
> 3. MAX_SKB_FRAGS exceeded: Free skb, cleanup and drop packet
> 4. alloc_page fails for frag: retry page allocation for frag w/o freeing skb
>
> Of these 1] and 3] can also happen in xsk_build_skb_zerocopy() and the
> error returned is either -EOVERFLOW or something else and the same
> error handling needs to be done.
That would be helpful to have it included in commit message.
>
> > I feel like we are trying to share some state between xsk_build_skb and
> > xsk_build_skb_zerocopy which we really shouldn't share. So how about
> > we try to have a separate cleanup path in xsk_build_skb_zerocopy?
> >
>
> The only things that are common between *copy and *zerocopy paths are
> setting the skb headers (destructor/args, mark, priority) and error handling.
>
> While we can potentially split out the paths into independent functions, the
> same skb header settings and error handling will still need to be duplicated in
> both functions.
>
> > Will something like the following (untested / uncompiled) work instead?
> >
> > IOW, ideally, xsk_build_skb should look like:
> >
> > if (dev->priv_flags & IFF_TX_SKB_NO_LINEAR) {
> > return xsk_build_skb_zerocopy(xs, desc);
> > } else {
> > return xsk_build_skb_copy(xs, desc);
> > /* ^^ current path that should really be a separate func */
> > }
IMHO this is way out of the scope for a fix. We can think later on about
cleaning up these paths.
> >
Powered by blists - more mailing lists