[bug report] Bluetooth: ISO: Add broadcast support

[bug report] Bluetooth: ISO: Add broadcast support

[Dan Carpenter]

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