| Message ID | 20220812055249.8037-1-palmer@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth: L2CAP: Elide a string overflow warning | expand |
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") The following commit fixes a similar problem (added the NULL check on line 1996): 332f1795ca20 ("Bluetooth: L2CAP: Fix l2cap_global_chan_by_psm regression") > In file included from /scratch/merges/ko-linux-next/linux/include/linux/string.h:253, > from /scratch/merges/ko-linux-next/linux/include/linux/bitmap.h:11, > from /scratch/merges/ko-linux-next/linux/include/linux/cpumask.h:12, > from /scratch/merges/ko-linux-next/linux/include/linux/mm_types_task.h:14, > from /scratch/merges/ko-linux-next/linux/include/linux/mm_types.h:5, > from /scratch/merges/ko-linux-next/linux/include/linux/buildid.h:5, > from /scratch/merges/ko-linux-next/linux/include/linux/module.h:14, > from /scratch/merges/ko-linux-next/linux/net/bluetooth/l2cap_core.c:31: > 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-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-next/linux/net/bluetooth/l2cap_core.c:2004: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); > | ^~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> Tested-by: Siddh Raman Pant <code@siddh.me> > --- > 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; > + } > } > } >
> -----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; > > + } > > } > > } > > >
On Tue, 30 Aug 2022 01:21:58 +0530 Elliott, Robert (Servers) wrote: > > -----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: You are correct, thanks for clarifying. Sorry for getting it reversed. So I think this patch can be modified to just introduce back the escape clause rather than having an extra indentation. Thanks, Siddh
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; + } } }