Re: [PATCH bluetooth] Bluetooth: eir: Fix stack OOB write in eir_create_adv_data()

[Weiming Shi <bestswngs@gmail.com>]

On 26-06-01 13:38, Luiz Augusto von Dentz wrote:
> Hi Xiang,
> 
> On Mon, Jun 1, 2026 at 12:22 PM Xiang Mei <bestswngs@gmail.com> wrote:
> >
> > From: Weiming Shi <bestswngs@gmail.com>
> >
> > eir_create_adv_data() builds the advertising data into a fixed-size
> > buffer of "size" bytes (31 for the legacy path in hci_set_adv_data_sync()).
> > It may prepend a 3-byte "Flags" AD structure, but the per-instance
> > advertising data is then copied without checking that it still fits:
> >
> > skip_flags:
> >         if (adv) {
> >                 memcpy(ptr, adv->adv_data, adv->adv_data_len);
> >
> > The "Flags" structure is added whenever BR/EDR is disabled
> > (LE_AD_NO_BREDR), which is the normal state for an LE-only controller.
> > However, the mgmt length validator tlv_data_max_len() only reserves
> > those 3 bytes when the user-supplied adv_flags carries a managed-flags
> > bit (DISCOV / LIMITED_DISCOV / MANAGED_FLAGS). Adding an instance with
> > flags == 0 reserves nothing, so adv_data_len is accepted up to the full
> > buffer size. At advertise time the 3 flag bytes are still prepended,
> > and the subsequent memcpy() writes 3 + adv_data_len bytes into the
> > size-byte buffer, overflowing it by the attacker-controlled tail of
> > adv_data:
> >
> >   BUG: KASAN: stack-out-of-bounds in eir_create_adv_data (net/bluetooth/eir.c:302)
> >   Write of size 31 at addr ffff88800b7e7bac by task kworker/u9:0/51
> >   Workqueue: hci0 hci_cmd_sync_work
> >    __asan_memcpy (mm/kasan/shadow.c:106)
> >    eir_create_adv_data (net/bluetooth/eir.c:302)
> >    hci_update_adv_data_sync (net/bluetooth/hci_sync.c:1689)
> >    hci_schedule_adv_instance_sync (net/bluetooth/hci_sync.c:2015)
> >    hci_cmd_sync_work (net/bluetooth/hci_sync.c:332)
> >   This frame has 1 object:
> >    [32, 64) 'cp'
> 
> Add the btmon trace of the MGMT command when this is triggered, and
> explaing how the advertisement was created, as with bluetoothd?
> 
> > The inconsistency dates back to when the managed "Flags" field was first
> > added to the Add Advertising path: the prepended LE_AD_NO_BREDR flag does
> > not depend on the user-supplied adv_flags, but tlv_data_is_valid() only
> > reserved room when MGMT_ADV_FLAG_DISCOV was set. Commit 47c03902269a
> > ("Bluetooth: eir: Fix possible crashes on eir_create_adv_data") later
> > added the "size" argument and bounds-checked the "Flags" and "Tx Power"
> > AD structures, but left this copy unguarded. Guard it the same way so
> > the data is only copied when it fits.
> >
> > The bug is reachable by a local user with CAP_NET_ADMIN that owns an
> > LE-only controller using the legacy advertising path.
> >
> > Fixes: b44133ff03be ("Bluetooth: Support the "discoverable" adv flag")
> > Reported-by: Xiang Mei <xmei5@asu.edu>
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> > ---
> >  net/bluetooth/eir.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/bluetooth/eir.c b/net/bluetooth/eir.c
> > index 3f72111ba651..e574f8f61e16 100644
> > --- a/net/bluetooth/eir.c
> > +++ b/net/bluetooth/eir.c
> > @@ -297,7 +297,7 @@ u8 eir_create_adv_data(struct hci_dev *hdev, u8 instance, u8 *ptr, u8 size)
> >         }
> >
> >  skip_flags:
> > -       if (adv) {
> > +       if (adv && ad_len + adv->adv_data_len <= size) {
> 
> So we have 2 options: 1) Don't add flags if there is no space, or 2)
> Don't add the user provided data. We should probably choose option 1,
> not option 2 since option 2 probably means the advertisement is
> useless.
> 
> >                 memcpy(ptr, adv->adv_data, adv->adv_data_len);
> >                 ad_len += adv->adv_data_len;
> >                 ptr += adv->adv_data_len;
> > --
> > 2.43.0
> >
> 
> 
> -- 
> Luiz Augusto von Dentz

Thanks. Agreed on option 1. I'll send v2 implementing that shortly.