linux-kernel - Re: [PATCH AUTOSEL 6.9 09/22] bluetooth/l2cap: sync sock recv cb and release

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]

Message-ID: <CABBYNZLzf2x6cScmjGv2Rxk-i3F9=QKVWosrSEBgmHBdHqOWtg@mail.gmail.com>
Date: Tue, 16 Jul 2024 15:06:01 -0400
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Pauli Virtanen <pav@....fi>
Cc: Sasha Levin <sashal@...nel.org>, linux-kernel@...r.kernel.org, stable@...r.kernel.org, 
	Edward Adam Davis <eadavis@...com>, syzbot+b7f6f8c9303466e16c8a@...kaller.appspotmail.com, 
	Luiz Augusto von Dentz <luiz.von.dentz@...el.com>, marcel@...tmann.org, johan.hedberg@...il.com, 
	linux-bluetooth@...r.kernel.org
Subject: Re: [PATCH AUTOSEL 6.9 09/22] bluetooth/l2cap: sync sock recv cb and release

Hi Pauli,

On Tue, Jul 16, 2024 at 3:00 PM Pauli Virtanen <pav@....fi> wrote:
>
> Hi,
>
> ti, 2024-07-16 kello 10:24 -0400, Sasha Levin kirjoitti:
> > From: Edward Adam Davis <eadavis@...com>
> >
> > [ Upstream commit 89e856e124f9ae548572c56b1b70c2255705f8fe ]
>
> This one needed an additional fixup that I don't see AUTOSEL picked up,
> otherwise it results to a worse regression:
>
> https://lore.kernel.org/linux-bluetooth/20240624134637.3790278-1-luiz.dentz@gmail.com/
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f1a8f402f13f94263cf349216c257b2985100927
>
>
> Looks like f1a8f402f13f94263cf349216c257b2985100927 also contains other
> changes not related to this patch, seems like
> https://lore.kernel.org/linux-bluetooth/20240624144911.3817479-1-luiz.dentz@gmail.com/
> was squashed.

Yep, it seems I messed them up while doing the pull-request and 2
commits were merged together, not sure if we can rebase them now that
are in Linus tree, anyway for stable it would be better to unmerge
them if possible.

> > The problem occurs between the system call to close the sock and hci_rx_work,
> > where the former releases the sock and the latter accesses it without lock protection.
> >
> >            CPU0                       CPU1
> >            ----                       ----
> >            sock_close                 hci_rx_work
> >          l2cap_sock_release         hci_acldata_packet
> >          l2cap_sock_kill            l2cap_recv_frame
> >          sk_free                    l2cap_conless_channel
> >                                     l2cap_sock_recv_cb
> >
> > If hci_rx_work processes the data that needs to be received before the sock is
> > closed, then everything is normal; Otherwise, the work thread may access the
> > released sock when receiving data.
> >
> > Add a chan mutex in the rx callback of the sock to achieve synchronization between
> > the sock release and recv cb.
> >
> > Sock is dead, so set chan data to NULL, avoid others use invalid sock pointer.
> >
> > Reported-and-tested-by: syzbot+b7f6f8c9303466e16c8a@...kaller.appspotmail.com
> > Signed-off-by: Edward Adam Davis <eadavis@...com>
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@...el.com>
> > Signed-off-by: Sasha Levin <sashal@...nel.org>
> > ---
> >  net/bluetooth/l2cap_sock.c | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index 8645461d45e81..64827e553d638 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -1239,6 +1239,10 @@ static void l2cap_sock_kill(struct sock *sk)
> >
> >       BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state));
> >
> > +     /* Sock is dead, so set chan data to NULL, avoid other task use invalid
> > +      * sock pointer.
> > +      */
> > +     l2cap_pi(sk)->chan->data = NULL;
> >       /* Kill poor orphan */
> >
> >       l2cap_chan_put(l2cap_pi(sk)->chan);
> > @@ -1481,12 +1485,25 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
> >
> >  static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
> >  {
> > -     struct sock *sk = chan->data;
> > -     struct l2cap_pinfo *pi = l2cap_pi(sk);
> > +     struct sock *sk;
> > +     struct l2cap_pinfo *pi;
> >       int err;
> >
> > -     lock_sock(sk);
> > +     /* To avoid race with sock_release, a chan lock needs to be added here
> > +      * to synchronize the sock.
> > +      */
> > +     l2cap_chan_hold(chan);
> > +     l2cap_chan_lock(chan);
> > +     sk = chan->data;
> >
> > +     if (!sk) {
> > +             l2cap_chan_unlock(chan);
> > +             l2cap_chan_put(chan);
> > +             return -ENXIO;
> > +     }
> > +
> > +     pi = l2cap_pi(sk);
> > +     lock_sock(sk);
> >       if (chan->mode == L2CAP_MODE_ERTM && !list_empty(&pi->rx_busy)) {
> >               err = -ENOMEM;
> >               goto done;
> > @@ -1535,6 +1552,8 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
> >
> >  done:
> >       release_sock(sk);
> > +     l2cap_chan_unlock(chan);
> > +     l2cap_chan_put(chan);
> >
> >       return err;
> >  }
>
> --
> Pauli Virtanen



-- 
Luiz Augusto von Dentz