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)); }
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
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
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?
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
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);
:(
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
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.
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
linux-stable-mirror@lists.linaro.org
-
Arnd Bergmann -
Arnd Bergmann -
Kees Cook -
Luiz Augusto von Dentz