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