net/bluetooth: fix erroneous use of bitmap_from_u64()

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 <sudipm.mukherjee@gmail.com>
Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 net/bluetooth/hci_core.c | 2 +-
 net/bluetooth/mgmt.c     | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

On Sun, Jun 5, 2022 at 9:25 AM Yury Norov <yury.norov@gmail.com> 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

On Sun, Jun 5, 2022 at 11:51 AM Linus Torvalds
<torvalds@linux-foundation.org> 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

Hi Linus,

On Sun, Jun 5, 2022 at 5:02 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sun, Jun 5, 2022 at 11:51 AM Linus Torvalds
> <torvalds@linux-foundation.org> 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?

On Mon, Jun 6, 2022 at 11:00 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> 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