* [PATCH] Bluetooth: mark bacmp() and bacpy() as __always_inline
@ 2023-10-09 13:48 Arnd Bergmann
2023-10-09 14:37 ` bluez.test.bot
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Arnd Bergmann @ 2023-10-09 13:48 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Lee, Chun-Yi
Cc: Arnd Bergmann, Kees Cook, Lee, Luiz Augusto von Dentz, stable,
Iulia Tanasescu, Wenjia Zhang, linux-bluetooth, netdev,
linux-kernel
From: Arnd Bergmann <arnd@arndb.de>
These functions are simple wrappers around memcmp() and memcpy(), which
contain compile-time checks for buffer overflow. Something in gcc-13 and
likely other versions makes this trigger a warning when the functions
are not inlined and the compiler misunderstands the buffer length:
In file included from net/bluetooth/hci_event.c:32:
In function 'bacmp',
inlined from 'hci_conn_request_evt' at net/bluetooth/hci_event.c:3276:7:
include/net/bluetooth/bluetooth.h:364:16: error: 'memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread]
364 | return memcmp(ba1, ba2, sizeof(bdaddr_t));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Use the __always_inline annotation to ensure that the helpers are
correctly checked. This has no effect on the actual correctness
of the code, but avoids the warning. Since the patch that introduced
the warning is marked for stable backports, this one should also
go that way to avoid introducing build regressions.
Fixes: d70e44fef8621 ("Bluetooth: Reject connection with the device which has same BD_ADDR")
Cc: Kees Cook <keescook@chromium.org>
Cc: Lee, Chun-Yi <jlee@suse.com>
Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: stable@vger.kernel.org
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
include/net/bluetooth/bluetooth.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 7ffa8c192c3f2..27ee1bf51c235 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -359,11 +359,11 @@ static inline bool bdaddr_type_is_le(u8 type)
#define BDADDR_NONE (&(bdaddr_t) {{0xff, 0xff, 0xff, 0xff, 0xff, 0xff}})
/* Copy, swap, convert BD Address */
-static inline int bacmp(const bdaddr_t *ba1, const bdaddr_t *ba2)
+static __always_inline int bacmp(const bdaddr_t *ba1, const bdaddr_t *ba2)
{
return memcmp(ba1, ba2, sizeof(bdaddr_t));
}
-static inline void bacpy(bdaddr_t *dst, const bdaddr_t *src)
+static __always_inline void bacpy(bdaddr_t *dst, const bdaddr_t *src)
{
memcpy(dst, src, sizeof(bdaddr_t));
}
--
2.39.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* RE: Bluetooth: mark bacmp() and bacpy() as __always_inline 2023-10-09 13:48 [PATCH] Bluetooth: mark bacmp() and bacpy() as __always_inline Arnd Bergmann @ 2023-10-09 14:37 ` bluez.test.bot 2023-10-09 15:36 ` [PATCH] " Kees Cook 2023-10-09 15:36 ` Arnd Bergmann 2 siblings, 0 replies; 10+ messages in thread From: bluez.test.bot @ 2023-10-09 14:37 UTC (permalink / raw) To: linux-bluetooth, arnd [-- Attachment #1: Type: text/plain, Size: 3257 bytes --] This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=791315 ---Test result--- Test Summary: CheckPatch FAIL 1.10 seconds GitLint FAIL 0.62 seconds SubjectPrefix PASS 0.13 seconds BuildKernel PASS 34.83 seconds CheckAllWarning PASS 38.35 seconds CheckSparse PASS 44.57 seconds CheckSmatch PASS 117.20 seconds BuildKernel32 PASS 33.68 seconds TestRunnerSetup PASS 513.96 seconds TestRunner_l2cap-tester PASS 31.42 seconds TestRunner_iso-tester PASS 52.83 seconds TestRunner_bnep-tester PASS 10.68 seconds TestRunner_mgmt-tester PASS 221.69 seconds TestRunner_rfcomm-tester PASS 16.26 seconds TestRunner_sco-tester PASS 19.74 seconds TestRunner_ioctl-tester PASS 18.41 seconds TestRunner_mesh-tester PASS 13.55 seconds TestRunner_smp-tester PASS 14.56 seconds TestRunner_userchan-tester PASS 11.22 seconds IncrementalBuild PASS 31.91 seconds Details ############################## Test: CheckPatch - FAIL Desc: Run checkpatch.pl script Output: Bluetooth: mark bacmp() and bacpy() as __always_inline WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?) #71: inlined from 'hci_conn_request_evt' at net/bluetooth/hci_event.c:3276:7: WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: ("Bluetooth: Reject connection with the device which has same BD_ADDR")' #82: Fixes: d70e44fef8621 ("Bluetooth: Reject connection with the device which has same BD_ADDR") total: 0 errors, 2 warnings, 13 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13413766.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. Use of uninitialized value $cid in concatenation (.) or string at /github/workspace/src/src/scripts/checkpatch.pl line 3228. ############################## Test: GitLint - FAIL Desc: Run gitlint Output: Bluetooth: mark bacmp() and bacpy() as __always_inline WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 13: B1 Line exceeds max length (125>80): "include/net/bluetooth/bluetooth.h:364:16: error: 'memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread]" --- Regards, Linux Bluetooth ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: mark bacmp() and bacpy() as __always_inline 2023-10-09 13:48 [PATCH] Bluetooth: mark bacmp() and bacpy() as __always_inline Arnd Bergmann 2023-10-09 14:37 ` bluez.test.bot @ 2023-10-09 15:36 ` Kees Cook 2023-10-09 15:36 ` Arnd Bergmann 2 siblings, 0 replies; 10+ messages in thread From: Kees Cook @ 2023-10-09 15:36 UTC (permalink / raw) To: Arnd Bergmann Cc: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Lee, Chun-Yi, Arnd Bergmann, Lee, Luiz Augusto von Dentz, stable, Iulia Tanasescu, Wenjia Zhang, linux-bluetooth, netdev, linux-kernel On Mon, Oct 09, 2023 at 03:48:19PM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > These functions are simple wrappers around memcmp() and memcpy(), which > contain compile-time checks for buffer overflow. Something in gcc-13 and > likely other versions makes this trigger a warning when the functions > are not inlined and the compiler misunderstands the buffer length: > > In file included from net/bluetooth/hci_event.c:32: > In function 'bacmp', > inlined from 'hci_conn_request_evt' at net/bluetooth/hci_event.c:3276:7: > include/net/bluetooth/bluetooth.h:364:16: error: 'memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread] > 364 | return memcmp(ba1, ba2, sizeof(bdaddr_t)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Use the __always_inline annotation to ensure that the helpers are > correctly checked. This has no effect on the actual correctness > of the code, but avoids the warning. Since the patch that introduced > the warning is marked for stable backports, this one should also > go that way to avoid introducing build regressions. Yes, good call. > > Fixes: d70e44fef8621 ("Bluetooth: Reject connection with the device which has same BD_ADDR") > Cc: Kees Cook <keescook@chromium.org> > Cc: Lee, Chun-Yi <jlee@suse.com> > Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > Cc: Marcel Holtmann <marcel@holtmann.org> > Cc: stable@vger.kernel.org > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Acked-by: Kees Cook <keescook@chromium.org> -Kees > --- > include/net/bluetooth/bluetooth.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > index 7ffa8c192c3f2..27ee1bf51c235 100644 > --- a/include/net/bluetooth/bluetooth.h > +++ b/include/net/bluetooth/bluetooth.h > @@ -359,11 +359,11 @@ static inline bool bdaddr_type_is_le(u8 type) > #define BDADDR_NONE (&(bdaddr_t) {{0xff, 0xff, 0xff, 0xff, 0xff, 0xff}}) > > /* Copy, swap, convert BD Address */ > -static inline int bacmp(const bdaddr_t *ba1, const bdaddr_t *ba2) > +static __always_inline int bacmp(const bdaddr_t *ba1, const bdaddr_t *ba2) > { > return memcmp(ba1, ba2, sizeof(bdaddr_t)); > } > -static inline void bacpy(bdaddr_t *dst, const bdaddr_t *src) > +static __always_inline void bacpy(bdaddr_t *dst, const bdaddr_t *src) > { > memcpy(dst, src, sizeof(bdaddr_t)); > } > -- > 2.39.2 > -- Kees Cook ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: mark bacmp() and bacpy() as __always_inline 2023-10-09 13:48 [PATCH] Bluetooth: mark bacmp() and bacpy() as __always_inline Arnd Bergmann 2023-10-09 14:37 ` bluez.test.bot 2023-10-09 15:36 ` [PATCH] " Kees Cook @ 2023-10-09 15:36 ` Arnd Bergmann 2023-10-09 16:02 ` Kees Cook 2 siblings, 1 reply; 10+ messages in thread From: Arnd Bergmann @ 2023-10-09 15:36 UTC (permalink / raw) To: Arnd Bergmann, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Lee, Chun-Yi Cc: Kees Cook, Luiz Augusto von Dentz, stable, Iulia Tanasescu, Wenjia Zhang, linux-bluetooth, Netdev, linux-kernel On Mon, Oct 9, 2023, at 15:48, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > These functions are simple wrappers around memcmp() and memcpy(), which > contain compile-time checks for buffer overflow. Something in gcc-13 and > likely other versions makes this trigger a warning when the functions > are not inlined and the compiler misunderstands the buffer length: > > In file included from net/bluetooth/hci_event.c:32: > In function 'bacmp', > inlined from 'hci_conn_request_evt' at > net/bluetooth/hci_event.c:3276:7: > include/net/bluetooth/bluetooth.h:364:16: error: 'memcmp' specified > bound 6 exceeds source size 0 [-Werror=stringop-overread] > 364 | return memcmp(ba1, ba2, sizeof(bdaddr_t)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Use the __always_inline annotation to ensure that the helpers are > correctly checked. This has no effect on the actual correctness > of the code, but avoids the warning. Since the patch that introduced > the warning is marked for stable backports, this one should also > go that way to avoid introducing build regressions. > > Fixes: d70e44fef8621 ("Bluetooth: Reject connection with the device > which has same BD_ADDR") > Cc: Kees Cook <keescook@chromium.org> > Cc: Lee, Chun-Yi <jlee@suse.com> > Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > Cc: Marcel Holtmann <marcel@holtmann.org> > Cc: stable@vger.kernel.org > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Sorry, I have to retract this, something went wrong on my testing and I now see the same problem in some configs regardless of whether the patch is applied or not. Arnd ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: mark bacmp() and bacpy() as __always_inline 2023-10-09 15:36 ` Arnd Bergmann @ 2023-10-09 16:02 ` Kees Cook 2023-10-09 18:23 ` Arnd Bergmann 0 siblings, 1 reply; 10+ messages in thread From: Kees Cook @ 2023-10-09 16:02 UTC (permalink / raw) To: Arnd Bergmann Cc: Arnd Bergmann, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Lee, Chun-Yi, Luiz Augusto von Dentz, stable, Iulia Tanasescu, Wenjia Zhang, linux-bluetooth, Netdev, linux-kernel On Mon, Oct 09, 2023 at 05:36:55PM +0200, Arnd Bergmann wrote: > On Mon, Oct 9, 2023, at 15:48, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > These functions are simple wrappers around memcmp() and memcpy(), which > > contain compile-time checks for buffer overflow. Something in gcc-13 and > > likely other versions makes this trigger a warning when the functions > > are not inlined and the compiler misunderstands the buffer length: > > > > In file included from net/bluetooth/hci_event.c:32: > > In function 'bacmp', > > inlined from 'hci_conn_request_evt' at > > net/bluetooth/hci_event.c:3276:7: > > include/net/bluetooth/bluetooth.h:364:16: error: 'memcmp' specified > > bound 6 exceeds source size 0 [-Werror=stringop-overread] > > 364 | return memcmp(ba1, ba2, sizeof(bdaddr_t)); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > Use the __always_inline annotation to ensure that the helpers are > > correctly checked. This has no effect on the actual correctness > > of the code, but avoids the warning. Since the patch that introduced > > the warning is marked for stable backports, this one should also > > go that way to avoid introducing build regressions. > > > > Fixes: d70e44fef8621 ("Bluetooth: Reject connection with the device > > which has same BD_ADDR") > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Lee, Chun-Yi <jlee@suse.com> > > Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > Cc: Marcel Holtmann <marcel@holtmann.org> > > Cc: stable@vger.kernel.org > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Sorry, I have to retract this, something went wrong on my > testing and I now see the same problem in some configs regardless > of whether the patch is applied or not. Perhaps turn them into macros instead? -- Kees Cook ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: mark bacmp() and bacpy() as __always_inline 2023-10-09 16:02 ` Kees Cook @ 2023-10-09 18:23 ` Arnd Bergmann 2023-10-09 19:48 ` Kees Cook 0 siblings, 1 reply; 10+ messages in thread From: Arnd Bergmann @ 2023-10-09 18:23 UTC (permalink / raw) To: Kees Cook Cc: Arnd Bergmann, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Chun-Yi Lee, Luiz Augusto von Dentz, stable, Iulia Tanasescu, Wenjia Zhang, linux-bluetooth, Netdev, linux-kernel On Mon, Oct 9, 2023, at 18:02, Kees Cook wrote: > On Mon, Oct 09, 2023 at 05:36:55PM +0200, Arnd Bergmann wrote: >> On Mon, Oct 9, 2023, at 15:48, Arnd Bergmann wrote: >> >> Sorry, I have to retract this, something went wrong on my >> testing and I now see the same problem in some configs regardless >> of whether the patch is applied or not. > > Perhaps turn them into macros instead? I just tried that and still see the problem even with the macro, so whatever gcc is doing must be a different issue. Maybe it has correctly found a codepath that triggers this? If you are able to help debug the issue better, see these defconfigs for examples: https://pastebin.com/raw/pC8Lnrn2 https://pastebin.com/raw/yb965unC Arnd ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: mark bacmp() and bacpy() as __always_inline 2023-10-09 18:23 ` Arnd Bergmann @ 2023-10-09 19:48 ` Kees Cook 2023-10-09 20:08 ` Arnd Bergmann 0 siblings, 1 reply; 10+ messages in thread From: Kees Cook @ 2023-10-09 19:48 UTC (permalink / raw) To: Arnd Bergmann Cc: Arnd Bergmann, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Chun-Yi Lee, Luiz Augusto von Dentz, stable, Iulia Tanasescu, Wenjia Zhang, linux-bluetooth, Netdev, linux-kernel On Mon, Oct 09, 2023 at 08:23:08PM +0200, Arnd Bergmann wrote: > On Mon, Oct 9, 2023, at 18:02, Kees Cook wrote: > > On Mon, Oct 09, 2023 at 05:36:55PM +0200, Arnd Bergmann wrote: > >> On Mon, Oct 9, 2023, at 15:48, Arnd Bergmann wrote: > >> > >> Sorry, I have to retract this, something went wrong on my > >> testing and I now see the same problem in some configs regardless > >> of whether the patch is applied or not. > > > > Perhaps turn them into macros instead? > > I just tried that and still see the problem even with the macro, > so whatever gcc is doing must be a different issue. Maybe it > has correctly found a codepath that triggers this? > > If you are able to help debug the issue better, > see these defconfigs for examples: > > https://pastebin.com/raw/pC8Lnrn2 > https://pastebin.com/raw/yb965unC This seems like a GCC bug. It is complaining about &hdev->bdaddr for some reason. This silences it: diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 6f4409b4c364..509e86b36576 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -3266,6 +3266,7 @@ static void hci_conn_request_evt(struct hci_dev *hdev, void *data, int mask = hdev->link_mode; struct inquiry_entry *ie; struct hci_conn *conn; + bdaddr_t a; __u8 flags = 0; bt_dev_dbg(hdev, "bdaddr %pMR type 0x%x", &ev->bdaddr, ev->link_type); @@ -3273,7 +3274,8 @@ static void hci_conn_request_evt(struct hci_dev *hdev, void *data, /* Reject incoming connection from device with same BD ADDR against * CVE-2020-26555 */ - if (!bacmp(&hdev->bdaddr, &ev->bdaddr)) { + a = hdev->bdaddr; + if (!bacmp(&a, &ev->bdaddr)) { bt_dev_dbg(hdev, "Reject connection with same BD_ADDR %pMR\n", &ev->bdaddr); hci_reject_conn(hdev, &ev->bdaddr); :( -- Kees Cook ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: mark bacmp() and bacpy() as __always_inline 2023-10-09 19:48 ` Kees Cook @ 2023-10-09 20:08 ` Arnd Bergmann 2023-10-09 20:15 ` Kees Cook 0 siblings, 1 reply; 10+ messages in thread From: Arnd Bergmann @ 2023-10-09 20:08 UTC (permalink / raw) To: Kees Cook Cc: Arnd Bergmann, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Chun-Yi Lee, Luiz Augusto von Dentz, stable, Iulia Tanasescu, Wenjia Zhang, linux-bluetooth, Netdev, linux-kernel On Mon, Oct 9, 2023, at 21:48, Kees Cook wrote: > On Mon, Oct 09, 2023 at 08:23:08PM +0200, Arnd Bergmann wrote: >> On Mon, Oct 9, 2023, at 18:02, Kees Cook wrote: >> > On Mon, Oct 09, 2023 at 05:36:55PM +0200, Arnd Bergmann wrote: >> >> On Mon, Oct 9, 2023, at 15:48, Arnd Bergmann wrote: >> >> >> >> Sorry, I have to retract this, something went wrong on my >> >> testing and I now see the same problem in some configs regardless >> >> of whether the patch is applied or not. >> > >> > Perhaps turn them into macros instead? >> >> I just tried that and still see the problem even with the macro, >> so whatever gcc is doing must be a different issue. Maybe it >> has correctly found a codepath that triggers this? >> >> If you are able to help debug the issue better, >> see these defconfigs for examples: >> >> https://pastebin.com/raw/pC8Lnrn2 >> https://pastebin.com/raw/yb965unC > > This seems like a GCC bug. It is complaining about &hdev->bdaddr for > some reason. This silences it: > > - if (!bacmp(&hdev->bdaddr, &ev->bdaddr)) { > + a = hdev->bdaddr; > + if (!bacmp(&a, &ev->bdaddr)) { Right, I see this addresses all instances. I tried another thing and this also seems to address them for me: --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -3273,7 +3273,7 @@ static void hci_conn_request_evt(struct hci_dev *hdev, void *data, /* Reject incoming connection from device with same BD ADDR against * CVE-2020-26555 */ - if (!bacmp(&hdev->bdaddr, &ev->bdaddr)) { + if (hdev && !bacmp(&hdev->bdaddr, &ev->bdaddr)) { bt_dev_dbg(hdev, "Reject connection with same BD_ADDR %pMR\n", &ev->bdaddr); hci_reject_conn(hdev, &ev->bdaddr); and also this one does the trick: --- a/include/net/bluetooth/bluetooth.h +++ b/include/net/bluetooth/bluetooth.h @@ -266,7 +266,7 @@ void bt_err_ratelimited(const char *fmt, ...); #define BT_DBG(fmt, ...) pr_debug(fmt "\n", ##__VA_ARGS__) #endif -#define bt_dev_name(hdev) ((hdev) ? (hdev)->name : "null") +#define bt_dev_name(hdev) ((hdev)->name) #define bt_dev_info(hdev, fmt, ...) \ BT_INFO("%s: " fmt, bt_dev_name(hdev), ##__VA_ARGS__) So what is actually going on is that the bt_dev_dbg() introduces the idea that hdev might be NULL because of the check. Arnd ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: mark bacmp() and bacpy() as __always_inline 2023-10-09 20:08 ` Arnd Bergmann @ 2023-10-09 20:15 ` Kees Cook 2023-10-10 0:01 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 10+ messages in thread From: Kees Cook @ 2023-10-09 20:15 UTC (permalink / raw) To: Arnd Bergmann Cc: Arnd Bergmann, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Chun-Yi Lee, Luiz Augusto von Dentz, stable, Iulia Tanasescu, Wenjia Zhang, linux-bluetooth, Netdev, linux-kernel On Mon, Oct 09, 2023 at 10:08:01PM +0200, Arnd Bergmann wrote: > On Mon, Oct 9, 2023, at 21:48, Kees Cook wrote: > > On Mon, Oct 09, 2023 at 08:23:08PM +0200, Arnd Bergmann wrote: > >> On Mon, Oct 9, 2023, at 18:02, Kees Cook wrote: > >> > On Mon, Oct 09, 2023 at 05:36:55PM +0200, Arnd Bergmann wrote: > >> >> On Mon, Oct 9, 2023, at 15:48, Arnd Bergmann wrote: > >> >> > >> >> Sorry, I have to retract this, something went wrong on my > >> >> testing and I now see the same problem in some configs regardless > >> >> of whether the patch is applied or not. > >> > > >> > Perhaps turn them into macros instead? > >> > >> I just tried that and still see the problem even with the macro, > >> so whatever gcc is doing must be a different issue. Maybe it > >> has correctly found a codepath that triggers this? > >> > >> If you are able to help debug the issue better, > >> see these defconfigs for examples: > >> > >> https://pastebin.com/raw/pC8Lnrn2 > >> https://pastebin.com/raw/yb965unC > > > > This seems like a GCC bug. It is complaining about &hdev->bdaddr for > > some reason. This silences it: > > > > - if (!bacmp(&hdev->bdaddr, &ev->bdaddr)) { > > + a = hdev->bdaddr; > > + if (!bacmp(&a, &ev->bdaddr)) { > > Right, I see this addresses all instances. I tried another thing > and this also seems to address them for me: > > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -3273,7 +3273,7 @@ static void hci_conn_request_evt(struct hci_dev *hdev, void *data, > /* Reject incoming connection from device with same BD ADDR against > * CVE-2020-26555 > */ > - if (!bacmp(&hdev->bdaddr, &ev->bdaddr)) { > + if (hdev && !bacmp(&hdev->bdaddr, &ev->bdaddr)) { > bt_dev_dbg(hdev, "Reject connection with same BD_ADDR %pMR\n", > &ev->bdaddr); > hci_reject_conn(hdev, &ev->bdaddr); > > and also this one does the trick: > > --- a/include/net/bluetooth/bluetooth.h > +++ b/include/net/bluetooth/bluetooth.h > @@ -266,7 +266,7 @@ void bt_err_ratelimited(const char *fmt, ...); > #define BT_DBG(fmt, ...) pr_debug(fmt "\n", ##__VA_ARGS__) > #endif > > -#define bt_dev_name(hdev) ((hdev) ? (hdev)->name : "null") > +#define bt_dev_name(hdev) ((hdev)->name) > > #define bt_dev_info(hdev, fmt, ...) \ > BT_INFO("%s: " fmt, bt_dev_name(hdev), ##__VA_ARGS__) > > So what is actually going on is that the bt_dev_dbg() introduces > the idea that hdev might be NULL because of the check. Oh thank you for finding that. Yeah, it looked to me like it thought hdev was NULL, but I couldn't find where. :) I think the best work-around here is your "hdev && " addition. -- Kees Cook ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: mark bacmp() and bacpy() as __always_inline 2023-10-09 20:15 ` Kees Cook @ 2023-10-10 0:01 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 10+ messages in thread From: Luiz Augusto von Dentz @ 2023-10-10 0:01 UTC (permalink / raw) To: Kees Cook Cc: Arnd Bergmann, Arnd Bergmann, Marcel Holtmann, Johan Hedberg, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Chun-Yi Lee, Luiz Augusto von Dentz, stable, Iulia Tanasescu, Wenjia Zhang, linux-bluetooth, Netdev, linux-kernel Hi, On Mon, Oct 9, 2023 at 1:15 PM Kees Cook <keescook@chromium.org> wrote: > > On Mon, Oct 09, 2023 at 10:08:01PM +0200, Arnd Bergmann wrote: > > On Mon, Oct 9, 2023, at 21:48, Kees Cook wrote: > > > On Mon, Oct 09, 2023 at 08:23:08PM +0200, Arnd Bergmann wrote: > > >> On Mon, Oct 9, 2023, at 18:02, Kees Cook wrote: > > >> > On Mon, Oct 09, 2023 at 05:36:55PM +0200, Arnd Bergmann wrote: > > >> >> On Mon, Oct 9, 2023, at 15:48, Arnd Bergmann wrote: > > >> >> > > >> >> Sorry, I have to retract this, something went wrong on my > > >> >> testing and I now see the same problem in some configs regardless > > >> >> of whether the patch is applied or not. > > >> > > > >> > Perhaps turn them into macros instead? > > >> > > >> I just tried that and still see the problem even with the macro, > > >> so whatever gcc is doing must be a different issue. Maybe it > > >> has correctly found a codepath that triggers this? > > >> > > >> If you are able to help debug the issue better, > > >> see these defconfigs for examples: > > >> > > >> https://pastebin.com/raw/pC8Lnrn2 > > >> https://pastebin.com/raw/yb965unC > > > > > > This seems like a GCC bug. It is complaining about &hdev->bdaddr for > > > some reason. This silences it: > > > > > > - if (!bacmp(&hdev->bdaddr, &ev->bdaddr)) { > > > + a = hdev->bdaddr; > > > + if (!bacmp(&a, &ev->bdaddr)) { > > > > Right, I see this addresses all instances. I tried another thing > > and this also seems to address them for me: > > > > --- a/net/bluetooth/hci_event.c > > +++ b/net/bluetooth/hci_event.c > > @@ -3273,7 +3273,7 @@ static void hci_conn_request_evt(struct hci_dev *hdev, void *data, > > /* Reject incoming connection from device with same BD ADDR against > > * CVE-2020-26555 > > */ > > - if (!bacmp(&hdev->bdaddr, &ev->bdaddr)) { > > + if (hdev && !bacmp(&hdev->bdaddr, &ev->bdaddr)) { > > bt_dev_dbg(hdev, "Reject connection with same BD_ADDR %pMR\n", > > &ev->bdaddr); > > hci_reject_conn(hdev, &ev->bdaddr); > > > > and also this one does the trick: > > > > --- a/include/net/bluetooth/bluetooth.h > > +++ b/include/net/bluetooth/bluetooth.h > > @@ -266,7 +266,7 @@ void bt_err_ratelimited(const char *fmt, ...); > > #define BT_DBG(fmt, ...) pr_debug(fmt "\n", ##__VA_ARGS__) > > #endif > > > > -#define bt_dev_name(hdev) ((hdev) ? (hdev)->name : "null") > > +#define bt_dev_name(hdev) ((hdev)->name) > > > > #define bt_dev_info(hdev, fmt, ...) \ > > BT_INFO("%s: " fmt, bt_dev_name(hdev), ##__VA_ARGS__) > > > > So what is actually going on is that the bt_dev_dbg() introduces > > the idea that hdev might be NULL because of the check. > > Oh thank you for finding that. Yeah, it looked to me like it thought > hdev was NULL, but I couldn't find where. :) > > I think the best work-around here is your "hdev && " addition. Perhaps we could something like: #define bt_dev_bacmp(hdev, bdaddr) ((hdev) ? bacmp(&(hdev)->bdaddr, bdaddr) : -EINVAL) Or the fact that we test for hdev makes the compiler assume it could NULL? If I recall correctly we did that because in some codepaths there is actually no hdev to use so it is passed as NULL. > -- > Kees Cook -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-10-10 0:02 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-09 13:48 [PATCH] Bluetooth: mark bacmp() and bacpy() as __always_inline Arnd Bergmann 2023-10-09 14:37 ` bluez.test.bot 2023-10-09 15:36 ` [PATCH] " Kees Cook 2023-10-09 15:36 ` Arnd Bergmann 2023-10-09 16:02 ` Kees Cook 2023-10-09 18:23 ` Arnd Bergmann 2023-10-09 19:48 ` Kees Cook 2023-10-09 20:08 ` Arnd Bergmann 2023-10-09 20:15 ` Kees Cook 2023-10-10 0:01 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox