net/bluetooth: fix erroneous use of bitmap_from_u64()

Commit Message

Yury Norov June 5, 2022, 4:25 p.m. UTC

The commit 0a97953fd221 ("lib: add bitmap_{from,to}_arr64") changed
implementation of bitmap_from_u64(), so that it doesn't typecast
argument to u64, and actually dereferences memory.

With that change, compiler spotted few places in bluetooth code
where bitmap_from_u64 is called for 32-bit variable.

As reported by Sudip Mukherjee:

"arm allmodconfig" fails with the error:

In file included from ./include/linux/string.h:253,
                 from ./include/linux/bitmap.h:11,
                 from ./include/linux/cpumask.h:12,
                 from ./include/linux/smp.h:13,
                 from ./include/linux/lockdep.h:14,
                 from ./include/linux/mutex.h:17,
                 from ./include/linux/rfkill.h:35,
                 from net/bluetooth/hci_core.c:29:
In function 'fortify_memcpy_chk',
    inlined from 'bitmap_copy' at ./include/linux/bitmap.h:254:2,
    inlined from 'bitmap_copy_clear_tail' at ./include/linux/bitmap.h:263:2,
    inlined from 'bitmap_from_u64' at ./include/linux/bitmap.h:540:2,
    inlined from 'hci_bdaddr_list_add_with_flags' at net/bluetooth/hci_core.c:2156:2:
./include/linux/fortify-string.h:344:25: error: call to '__write_overflow_field' declared with attribute warning:
+detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
  344 |                         __write_overflow_field(p_size_field, size);

And, "csky allmodconfig" fails with the error:

In file included from ./include/linux/cpumask.h:12,
                 from ./include/linux/mm_types_task.h:14,
                 from ./include/linux/mm_types.h:5,
                 from ./include/linux/buildid.h:5,
                 from ./include/linux/module.h:14,
                 from net/bluetooth/mgmt.c:27:
In function 'bitmap_copy',
    inlined from 'bitmap_copy_clear_tail' at ./include/linux/bitmap.h:263:2,
    inlined from 'bitmap_from_u64' at ./include/linux/bitmap.h:540:2,
    inlined from 'set_device_flags' at net/bluetooth/mgmt.c:4534:4:
./include/linux/bitmap.h:254:9: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] of object 'flags'
+with type 'long unsigned int[1]' [-Werror=array-bounds]
  254 |         memcpy(dst, src, len);
      |         ^~~~~~~~~~~~~~~~~~~~~
In file included from ./include/linux/kasan-checks.h:5,
                 from ./include/asm-generic/rwonce.h:26,
                 from ./arch/csky/include/generated/asm/rwonce.h:1,
                 from ./include/linux/compiler.h:248,
                 from ./include/linux/build_bug.h:5,
                 from ./include/linux/container_of.h:5,
                 from ./include/linux/list.h:5,
                 from ./include/linux/module.h:12,
                 from net/bluetooth/mgmt.c:27:
net/bluetooth/mgmt.c: In function 'set_device_flags':
net/bluetooth/mgmt.c:4532:40: note: 'flags' declared here
 4532 |                         DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS);
      |                                        ^~~~~
./include/linux/types.h:11:23: note: in definition of macro 'DECLARE_BITMAP'
   11 |         unsigned long name[BITS_TO_LONGS(bits)]

Fix it by replacing bitmap_from_u64 with bitmap_from_arr32.

Reported-by: Sudip Mukherjee <[e-mail]>
Signed-off-by: Yury Norov <[e-mail]>
---
 net/bluetooth/hci_core.c | 2 +-
 net/bluetooth/mgmt.c     | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Linus Torvalds June 5, 2022, 4:34 p.m. UTC | #1

On Sun, Jun 5, 2022 at 9:25 AM Yury Norov <[e-mail]> wrote:
>
> The commit 0a97953fd221 ("lib: add bitmap_{from,to}_arr64") changed
> implementation of bitmap_from_u64(), so that it doesn't typecast
> argument to u64, and actually dereferences memory.

Gaah.

That code shouldn't use DECLARE_BITMAP() at all, it should just use

    struct bdaddr_list_with_flags {
            ..
            unsigned long flags;
    };

and then use '&br_params->flags' when it nneds the actual atomic
'set_bit()' things and friends, and then when it copies the flags
around it should just use 'flags' as an integer value.

The bitmap functions are literally defined to work as "bit N in a set
of 'unsigned long'" exactly so that you can do that mixing of values
and bit operations, and not have to worry about insane architectures
that do big-endian bit ordering or things like that.

Using a 'bitmap' as if it's some bigger or potentially variable-sized
thing for this kind of flags usage is crazy, when the code already
does

  /* Make sure number of flags doesn't exceed sizeof(current_flags) */
  static_assert(__HCI_CONN_NUM_FLAGS < 32);

because other parts are limited to 32 bits.

I wonder how painful it would be to just fix that odd type mistake.

                  Linus

[e-mail] June 5, 2022, 5:14 p.m. UTC | #2

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=647461

---Test result---

Test Summary:
CheckPatch                    FAIL      1.95 seconds
GitLint                       FAIL      1.03 seconds
SubjectPrefix                 FAIL      0.87 seconds
BuildKernel                   PASS      36.71 seconds
BuildKernel32                 PASS      32.83 seconds
Incremental Build with patchesPASS      44.05 seconds
TestRunner: Setup             PASS      546.12 seconds
TestRunner: l2cap-tester      PASS      18.98 seconds
TestRunner: bnep-tester       PASS      6.98 seconds
TestRunner: mgmt-tester       PASS      113.33 seconds
TestRunner: rfcomm-tester     PASS      10.88 seconds
TestRunner: sco-tester        PASS      10.46 seconds
TestRunner: smp-tester        PASS      10.53 seconds
TestRunner: userchan-tester   PASS      7.30 seconds

Details
##############################
Test: CheckPatch - FAIL - 1.95 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
net/bluetooth: fix erroneous use of bitmap_from_u64()\WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#117: 
    inlined from 'bitmap_copy_clear_tail' at ./include/linux/bitmap.h:263:2,

total: 0 errors, 1 warnings, 0 checks, 33 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/12869809.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.


##############################
Test: GitLint - FAIL - 1.03 seconds
Run gitlint with rule in .gitlint
net/bluetooth: fix erroneous use of bitmap_from_u64()
26: B1 Line exceeds max length (85>80): "    inlined from 'hci_bdaddr_list_add_with_flags' at net/bluetooth/hci_core.c:2156:2:"
27: B1 Line exceeds max length (113>80): "./include/linux/fortify-string.h:344:25: error: call to '__write_overflow_field' declared with attribute warning:"
28: B1 Line exceeds max length (107>80): "+detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]"
43: B1 Line exceeds max length (115>80): "./include/linux/bitmap.h:254:9: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] of object 'flags'"


##############################
Test: SubjectPrefix - FAIL - 0.87 seconds
Check subject contains "Bluetooth" prefix
"Bluetooth: " is not specified in the subject



---
Regards,
Linux Bluetooth

Linus Torvalds June 5, 2022, 6:51 p.m. UTC | #3

On Sun, Jun 5, 2022 at 9:34 AM Linus Torvalds
<[e-mail]> wrote:
>
> That code shouldn't use DECLARE_BITMAP() at all, it should just use
>
>     struct bdaddr_list_with_flags {
>             ..
>             unsigned long flags;
>     };
>
> and then use '&br_params->flags' when it needs the actual atomic
> 'set_bit()' things and friends,

Actually, I'm not convinced those things should be atomic at all.

*Most* of the accesses to those connection flags seem to be with
hci_dev_lock() held, and the ones that aren't can't possibly depend on
atomicity since those things are currently copied around with random
other "copy bitmaps" functions.

So I think the bluetooth code would actually be much better off with
something like

    /* Bitmask of connection flags */
    enum hci_conn_flags {
        HCI_CONN_FLAG_REMOTE_WAKEUP = 1,
        HCI_CONN_FLAG_DEVICE_PRIVACY = 2,
    };
    typedef u8 hci_conn_flags_t;

instead of playing games with DECLARE_BITMAP() and then using a mix of
atomic set_bit/clear_bit() and random non-atomic "copy bitmap values
around".

This attached patch builds cleanly for me, doing the above. But see a
few comments about those atomicity issues...

And no, it doesn't really need that new "hci_conn_flags_t", and it
could just use "u32" (which is what "current_flags" and
"supported_flags" use in the code), but those structures that contain
those connection flags do seem to have various other byte fields and
it would appear to pack better using just that simple one-byte type.

It *literally* just uses two bits in it, after all, and as mentioned,
the whole atomicity situation right now is very very dubious, so it
seems to make sense to do something like this.

Reactions?

                 Linus

Linus Torvalds June 5, 2022, 11:56 p.m. UTC | #4

On Sun, Jun 5, 2022 at 11:51 AM Linus Torvalds
<[e-mail]> wrote:
>
> *Most* of the accesses to those connection flags seem to be with
> hci_dev_lock() held, and the ones that aren't can't possibly depend on
> atomicity since those things are currently copied around with random
> other "copy bitmaps" functions.

I've committed that patch as commit e1cff7002b71 ("bluetooth: don't
use bitmaps for random flag accesses").

That basically ends up reverting

  a9a347655d22 ("Bluetooth: MGMT: Add conditions for setting
HCI_CONN_FLAG_REMOTE_WAKEUP")
  6126ffabba6b ("Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag")

which did horrible things, and would end up overwriting the end of the
bitmap allocation on 32-bit architectures.

Luiz, if the reason for the change to use a bitmap type was because of
some atomicity concerns, then you can do that by

 (a) change 'hci_conn_flags_t' to be an 'atomic_t' instead of a 'u8'

 (b) change the regular accesses to it to use 'atomic_read/write()'

 (c) change the "bitfield" operations to use 'atomic_or/andnot()'

but honestly, when it used to mix atomic ops
(set_bit/clear_bit/test_bit) with random non-atomic users
(bitmap_from_u64(), bitmap_to_arr32() etc) it was never atomic to
begin with.

Regardless, trying to use bitmaps for this was absolutely not the
right thing to ever do. It looks like gcc randomly started complaining
when 'bitmap_from_u64()' was changed, but it was buggy before that
too.

                 Linus

Luiz Augusto von Dentz June 7, 2022, 6 a.m. UTC | #5

Hi Linus,

On Sun, Jun 5, 2022 at 5:02 PM Linus Torvalds
<[e-mail]> wrote:
>
> On Sun, Jun 5, 2022 at 11:51 AM Linus Torvalds
> <[e-mail]> wrote:
> >
> > *Most* of the accesses to those connection flags seem to be with
> > hci_dev_lock() held, and the ones that aren't can't possibly depend on
> > atomicity since those things are currently copied around with random
> > other "copy bitmaps" functions.
>
> I've committed that patch as commit e1cff7002b71 ("bluetooth: don't
> use bitmaps for random flag accesses").
>
> That basically ends up reverting
>
>   a9a347655d22 ("Bluetooth: MGMT: Add conditions for setting
> HCI_CONN_FLAG_REMOTE_WAKEUP")
>   6126ffabba6b ("Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag")
>
> which did horrible things, and would end up overwriting the end of the
> bitmap allocation on 32-bit architectures.
>
> Luiz, if the reason for the change to use a bitmap type was because of
> some atomicity concerns, then you can do that by
>
>  (a) change 'hci_conn_flags_t' to be an 'atomic_t' instead of a 'u8'
>
>  (b) change the regular accesses to it to use 'atomic_read/write()'
>
>  (c) change the "bitfield" operations to use 'atomic_or/andnot()'
>
> but honestly, when it used to mix atomic ops
> (set_bit/clear_bit/test_bit) with random non-atomic users
> (bitmap_from_u64(), bitmap_to_arr32() etc) it was never atomic to
> begin with.
>
> Regardless, trying to use bitmaps for this was absolutely not the
> right thing to ever do. It looks like gcc randomly started complaining
> when 'bitmap_from_u64()' was changed, but it was buggy before that
> too.

Right, thanks for fixing it. About some of the changes perhaps we
should use BIT when declaring values in enum hci_conn_flags?

Linus Torvalds June 7, 2022, 6:49 p.m. UTC | #6

On Mon, Jun 6, 2022 at 11:00 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Right, thanks for fixing it. About some of the changes perhaps we
> should use BIT when declaring values in enum hci_conn_flags?

That sounds sane, although with just two flag values I'm not sure it matters.

But I guess it would document the fact that it's a bitmask, not an
ordinal value, and it looks like that header is already using BIT()
elsewhere so there are no new header file dependencies..

              Linus

Patch

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5abb2ca5b129..2de7e1ec4035 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2153,7 +2153,7 @@  int hci_bdaddr_list_add_with_flags(struct list_head *list, bdaddr_t *bdaddr,
 
 	bacpy(&entry->bdaddr, bdaddr);
 	entry->bdaddr_type = type;
-	bitmap_from_u64(entry->flags, flags);
+	bitmap_from_arr32(entry->flags, &flags, 32);
 
 	list_add(&entry->list, list);
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 74937a834648..b63025c70c2c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4519,7 +4519,8 @@  static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 							      cp->addr.type);
 
 		if (br_params) {
-			bitmap_from_u64(br_params->flags, current_flags);
+			bitmap_from_arr32(br_params->flags, &current_flags,
+					  __HCI_CONN_NUM_FLAGS);
 			status = MGMT_STATUS_SUCCESS;
 		} else {
 			bt_dev_warn(hdev, "No such BR/EDR device %pMR (0x%x)",
@@ -4531,7 +4532,7 @@  static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 		if (params) {
 			DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS);
 
-			bitmap_from_u64(flags, current_flags);
+			bitmap_from_arr32(flags, &current_flags, __HCI_CONN_NUM_FLAGS);
 
 			/* Devices using RPAs can only be programmed in the
 			 * acceptlist LL Privacy has been enable otherwise they
@@ -4546,7 +4547,7 @@  static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 				goto unlock;
 			}
 
-			bitmap_from_u64(params->flags, current_flags);
+			bitmap_from_arr32(params->flags, &current_flags, __HCI_CONN_NUM_FLAGS);
 			status = MGMT_STATUS_SUCCESS;
 
 			/* Update passive scan if HCI_CONN_FLAG_DEVICE_PRIVACY