Bluetooth: ISO: Add broadcast support

Hi Luiz,

Harshit Mogalapalli brought this memory corruption issue to me. Can you take a look? I don't know how to fix it.

The patch f764a6c2c1e4: "Bluetooth: ISO: Add broadcast support" from Mar 9, 2022, leads to the following Smatch static checker warning:

	net/bluetooth/iso.c:1282 iso_sock_getsockopt()
	error: copy_to_user() 'base' too small (252 vs 254)

That warning is because Smatch gets confused but in reviewing the code, it turns out that Smatch is correct (like a stopped clock which is correct by accident). The actual bug happens earlier in eir_append_service_data().

Step 1: iso_sock_setsockopt() sets ->base_len to 0-252

net/bluetooth/iso.c

  1208                  if (optlen > sizeof(iso_pi(sk)->base)) {
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  1209                          err = -EOVERFLOW;
  1210                          break;
  1211                  }
  1212  
  1213                  len = min_t(unsigned int, sizeof(iso_pi(sk)->base), optlen);
  1214  
  1215                  if (copy_from_sockptr(iso_pi(sk)->base, optval, len)) {
  1216                          err = -EFAULT;
  1217                          break;
  1218                  }
  1219  
  1220                  iso_pi(sk)->base_len = len;
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^

Step 2: iso_connect_bis() passes ->base_len to hci_connect_bis()

net/bluetooth/iso.c
235 static int iso_connect_bis(struct sock *sk) 236 {

   237          struct iso_conn *conn;
   238          struct hci_conn *hcon;
   239          struct hci_dev  *hdev;
   240          int err;
   241  
   242          BT_DBG("%pMR", &iso_pi(sk)->src);
   243  
   244          hdev = hci_get_route(&iso_pi(sk)->dst, &iso_pi(sk)->src,
   245                               iso_pi(sk)->src_type);
   246          if (!hdev)
   247                  return -EHOSTUNREACH;
   248  
   249          hci_dev_lock(hdev);
   250  
   251          if (!bis_capable(hdev)) {
   252                  err = -EOPNOTSUPP;
   253                  goto done;
   254          }
   255  
   256          /* Fail if out PHYs are marked as disabled */
   257          if (!iso_pi(sk)->qos.out.phy) {
   258                  err = -EINVAL;
   259                  goto done;
   260          }
   261  
   262          hcon = hci_connect_bis(hdev, &iso_pi(sk)->dst, iso_pi(sk)->dst_type,
   263                                 &iso_pi(sk)->qos, iso_pi(sk)->base_len,
                                                         ^^^^^^^^^^^^^^^^^^^^^

264 iso_pi(sk)->base);
265 if (IS_ERR(hcon)) {
266 err = PTR_ERR(hcon);

Step 3: hci_connect_bis() passes base_len to eir_append_service_data(). The buffer here is ->le_per_adv_data which is also size 252 bytes.

net/bluetooth/hci_conn.c
2058 /* Add Basic Announcement into Peridic Adv Data if BASE is set */
2059 if (base_len && base) {
2060 base_len = eir_append_service_data(conn->le_per_adv_data, 0,

                                                           ^^^^^^^^^^^^^^^^^^^^^
  2061                                                     0x1851, base, base_len);
                                                                         ^^^^^^^^

2062 conn->le_per_adv_data_len = base_len;
2063 }

Step 4: memory corruption in eir_append_service_data()

net/bluetooth/eir.c

    69  u8 eir_append_service_data(u8 *eir, u16 eir_len, u16 uuid, u8 *data,
    70                             u8 data_len)
    71  {
    72          eir[eir_len++] = sizeof(u8) + sizeof(uuid) + data_len;
    73          eir[eir_len++] = EIR_SERVICE_DATA;
    74          put_unaligned_le16(uuid, &eir[eir_len]);
    75          eir_len += sizeof(uuid);
    76          memcpy(&eir[eir_len], data, data_len);
                            ^^^^^^^         ^^^^^^^^
    77          eir_len += data_len;
    78  
    79          return eir_len;
    80  }

The "eir" buffer has 252 bytes and data_len is 252 but we do a memcpy() to &eir[4] so this can corrupt 4 bytes beyond the end of the buffer.

If you look back at the caller it sets conn->le_per_adv_data_len to a max of 4 + 252 = 256 but truncated to 255. This eventually gets passed to iso_sock_getsockopt() leading to a read overflow. But the first part of the bug is in eir_append_service_data().

regards,
dan carpenter