Messages in this thread Patch in this message | | | From | "Elliott, Robert (Servers)" <> | Subject | RE: [PATCH] Bluetooth: L2CAP: Elide a string overflow warning | Date | Mon, 29 Aug 2022 19:51:58 +0000 |
| |
> -----Original Message----- > From: Siddh Raman Pant <code@siddh.me> > Sent: Thursday, August 25, 2022 6:01 AM > To: palmer@rivosinc.com > Cc: davem@davemloft.net; edumazet@google.com; johan.hedberg@gmail.com; > kuba@kernel.org; linux-bluetooth@vger.kernel.org; linux- > kernel@vger.kernel.org; linux@rivosinc.com; luiz.dentz@gmail.com; > marcel@holtmann.org; netdev@vger.kernel.org; pabeni@redhat.com > Subject: Re: [PATCH] Bluetooth: L2CAP: Elide a string overflow warning > > On Fri, 12 Aug 2022 11:22:49 +0530 Palmer Dabbelt wrote: > > From: Palmer Dabbelt <palmer@rivosinc.com> > > > > Without this I get a string op warning related to copying from a > > possibly NULL pointer. I think the warning is spurious, but it's > > tripping up allmodconfig. > > I think it is not spurious, and is due to the following commit: > d0be8347c623 ("Bluetooth: L2CAP: Fix use-after-free caused by l2cap_chan_put")
That commit was OK - it added an "if (!c) continue" to handle if the value c is changed to NULL. > The following commit fixes a similar problem (added the NULL check on line > 1996): > 332f1795ca20 ("Bluetooth: L2CAP: Fix l2cap_global_chan_by_psm regression")
That commit wiped out the "if (!c) continue" path escape clause from the previous patch, introducing a path back to code that doesn't check for NULL:
- if (!c) - continue; - - read_unlock(&chan_list_lock); - return c; + if (c) { + read_unlock(&chan_list_lock); + return c; + } } src_any = !bacmp(&c->src, BDADDR_ANY); dst_any = !bacmp(&c->dst, BDADDR_ANY);
> > inlined from 'l2cap_global_chan_by_psm' at /scratch/merges/ko-linux- > next/linux/net/bluetooth/l2cap_core.c:2003:15: > > /scratch/merges/ko-linux-next/linux/include/linux/fortify-string.h:44:33: > error: '__builtin_memcmp' specified bound 6 exceeds source size 0 [- > Werror=stringop-overread] > > 44 | #define __underlying_memcmp __builtin_memcmp > > | ^ > > /scratch/merges/ko-linux-next/linux/include/linux/fortify-string.h:420:16: > note: in expansion of macro '__underlying_memcmp' > > 420 | return __underlying_memcmp(p, q, size); > > | ^~~~~~~~~~~~~~~~~~~ > > In function 'memcmp', > > inlined from 'bacmp' at /scratch/merges/ko-linux- > next/linux/include/net/bluetooth/bluetooth.h:347:9, > > inlined from 'l2cap_global_chan_by_psm' at /scratch/merges/ko-linux- ... > > > > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> > > Tested-by: Siddh Raman Pant <code@siddh.me>
This patch is necessary to get a successful cross-compile of 6.0-rc3 allmodconfig for ARCH=mips on x86.
Tested-by: Robert Elliott <elliott@hpe.com>
I suggest you label this patch as: Fixes: 332f1795ca20 ("Bluetooth: L2CAP: Fix l2cap_global_chan_by_psm regression")
> > --- > > net/bluetooth/l2cap_core.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > > index cbe0cae73434..be7f47e52119 100644 > > --- a/net/bluetooth/l2cap_core.c > > +++ b/net/bluetooth/l2cap_core.c > > @@ -2000,11 +2000,13 @@ static struct l2cap_chan > *l2cap_global_chan_by_psm(int state, __le16 psm, > > } > > > > /* Closest match */ > > - src_any = !bacmp(&c->src, BDADDR_ANY); > > - dst_any = !bacmp(&c->dst, BDADDR_ANY); > > - if ((src_match && dst_any) || (src_any && dst_match) || > > - (src_any && dst_any)) > > - c1 = c; > > + if (c) { > > + src_any = !bacmp(&c->src, BDADDR_ANY); > > + dst_any = !bacmp(&c->dst, BDADDR_ANY); > > + if ((src_match && dst_any) || (src_any && > dst_match) || > > + (src_any && dst_any)) > > + c1 = c; > > + } > > } > > } > > >
| |