* [PATCH v3 1/2] Bluetooth: Fix not checking advertisement bondaries
@ 2020-10-24 0:22 Luiz Augusto von Dentz
2020-10-24 0:22 ` [PATCH v3 2/2] Bluetooth: A2MP: Fix not setting request ID Luiz Augusto von Dentz
2021-02-13 8:25 ` [PATCH v3 1/2] Bluetooth: Fix not checking advertisement bondaries Salvatore Bonaccorso
0 siblings, 2 replies; 3+ messages in thread
From: Luiz Augusto von Dentz @ 2020-10-24 0:22 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
When receiving advertisements check if the length is actually within
the skb, this also make use of skb_pull to advance on the skb->data
instead of a custom ptr that way skb->len shall always indicates how
much data is remaining and can be used to perform checks if there is
enough data to parse.
Fixes: a2ec905d1e160a33b2e210e45ad30445ef26ce0e ("Bluetooth: fix kernel oops in store_pending_adv_report")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
net/bluetooth/hci_event.c | 73 ++++++++++++++++++++++++++++++---------
1 file changed, 56 insertions(+), 17 deletions(-)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index a4c3703f2e94..6925c090a9e0 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -5599,24 +5599,41 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
{
u8 num_reports = skb->data[0];
- void *ptr = &skb->data[1];
hci_dev_lock(hdev);
+ skb_pull(skb, sizeof(num_reports));
+
while (num_reports--) {
- struct hci_ev_le_advertising_info *ev = ptr;
+ struct hci_ev_le_advertising_info *ev;
s8 rssi;
- if (ev->length <= HCI_MAX_AD_LENGTH) {
- rssi = ev->data[ev->length];
- process_adv_report(hdev, ev->evt_type, &ev->bdaddr,
- ev->bdaddr_type, NULL, 0, rssi,
- ev->data, ev->length, false);
- } else {
- bt_dev_err(hdev, "Dropping invalid advertising data");
+ if (skb->len < sizeof(*ev)) {
+ bt_dev_err(hdev, "Malformed advertising report");
+ break;
+ }
+
+ ev = (void *) skb->data;
+ skb_pull(skb, sizeof(*ev));
+
+ if (skb->len < ev->length || ev->length > HCI_MAX_AD_LENGTH) {
+ bt_dev_err(hdev, "Malformed advertising data");
+ break;
}
- ptr += sizeof(*ev) + ev->length + 1;
+ skb_pull(skb, ev->length);
+
+ if (skb->len < sizeof(rssi)) {
+ bt_dev_err(hdev, "Malformed advertising rssi");
+ break;
+ }
+
+ rssi = skb->data[0];
+ skb_pull(skb, sizeof(rssi));
+
+ process_adv_report(hdev, ev->evt_type, &ev->bdaddr,
+ ev->bdaddr_type, NULL, 0, rssi,
+ ev->data, ev->length, false);
}
hci_dev_unlock(hdev);
@@ -5669,15 +5686,31 @@ static u8 ext_evt_type_to_legacy(struct hci_dev *hdev, u16 evt_type)
static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
{
u8 num_reports = skb->data[0];
- void *ptr = &skb->data[1];
hci_dev_lock(hdev);
+ skb_pull(skb, sizeof(num_reports));
+
while (num_reports--) {
- struct hci_ev_le_ext_adv_report *ev = ptr;
+ struct hci_ev_le_ext_adv_report *ev;
u8 legacy_evt_type;
u16 evt_type;
+ if (skb->len < sizeof(*ev)) {
+ bt_dev_err(hdev, "Malformed ext advertising report");
+ break;
+ }
+
+ ev = (void *) skb->data;
+ skb_pull(skb, sizeof(*ev));
+
+ if (skb->len < ev->length || ev->length > HCI_MAX_AD_LENGTH) {
+ bt_dev_err(hdev, "Malformed ext advertising data");
+ break;
+ }
+
+ skb_pull(skb, ev->length);
+
evt_type = __le16_to_cpu(ev->evt_type);
legacy_evt_type = ext_evt_type_to_legacy(hdev, evt_type);
if (legacy_evt_type != LE_ADV_INVALID) {
@@ -5687,7 +5720,6 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
!(evt_type & LE_EXT_ADV_LEGACY_PDU));
}
- ptr += sizeof(*ev) + ev->length;
}
hci_dev_unlock(hdev);
@@ -5873,19 +5905,26 @@ static void hci_le_direct_adv_report_evt(struct hci_dev *hdev,
struct sk_buff *skb)
{
u8 num_reports = skb->data[0];
- void *ptr = &skb->data[1];
hci_dev_lock(hdev);
+ skb_pull(skb, sizeof(num_reports));
+
while (num_reports--) {
- struct hci_ev_le_direct_adv_info *ev = ptr;
+ struct hci_ev_le_direct_adv_info *ev;
+
+ if (skb->len < sizeof(*ev)) {
+ bt_dev_err(hdev, "Malformed direct advertising");
+ break;
+ }
+
+ ev = (void *) skb->data;
+ skb_pull(skb, sizeof(*ev));
process_adv_report(hdev, ev->evt_type, &ev->bdaddr,
ev->bdaddr_type, &ev->direct_addr,
ev->direct_addr_type, ev->rssi, NULL, 0,
false);
-
- ptr += sizeof(*ev);
}
hci_dev_unlock(hdev);
--
2.26.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH v3 2/2] Bluetooth: A2MP: Fix not setting request ID
2020-10-24 0:22 [PATCH v3 1/2] Bluetooth: Fix not checking advertisement bondaries Luiz Augusto von Dentz
@ 2020-10-24 0:22 ` Luiz Augusto von Dentz
2021-02-13 8:25 ` [PATCH v3 1/2] Bluetooth: Fix not checking advertisement bondaries Salvatore Bonaccorso
1 sibling, 0 replies; 3+ messages in thread
From: Luiz Augusto von Dentz @ 2020-10-24 0:22 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This fixes not resetting of the request ID when sending
A2MP_GETAMPASSOC_RSP.
Fixes: eddb7732119d53400f48a02536a84c509692faa8 ("Bluetooth: A2MP: Fix not initializing all members")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
net/bluetooth/a2mp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/a2mp.c b/net/bluetooth/a2mp.c
index da7fd7c8c2dc..7a1e0b785f45 100644
--- a/net/bluetooth/a2mp.c
+++ b/net/bluetooth/a2mp.c
@@ -381,10 +381,11 @@ static int a2mp_getampassoc_req(struct amp_mgr *mgr, struct sk_buff *skb,
hdev = hci_dev_get(req->id);
if (!hdev || hdev->amp_type == AMP_TYPE_BREDR || tmp) {
struct a2mp_amp_assoc_rsp rsp;
- rsp.id = req->id;
memset(&rsp, 0, sizeof(rsp));
+ rsp.id = req->id;
+
if (tmp) {
rsp.status = A2MP_STATUS_COLLISION_OCCURED;
amp_mgr_put(tmp);
--
2.26.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3 1/2] Bluetooth: Fix not checking advertisement bondaries
2020-10-24 0:22 [PATCH v3 1/2] Bluetooth: Fix not checking advertisement bondaries Luiz Augusto von Dentz
2020-10-24 0:22 ` [PATCH v3 2/2] Bluetooth: A2MP: Fix not setting request ID Luiz Augusto von Dentz
@ 2021-02-13 8:25 ` Salvatore Bonaccorso
1 sibling, 0 replies; 3+ messages in thread
From: Salvatore Bonaccorso @ 2021-02-13 8:25 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
On Fri, Oct 23, 2020 at 05:22:50PM -0700, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> When receiving advertisements check if the length is actually within
> the skb, this also make use of skb_pull to advance on the skb->data
> instead of a custom ptr that way skb->len shall always indicates how
> much data is remaining and can be used to perform checks if there is
> enough data to parse.
>
> Fixes: a2ec905d1e160a33b2e210e45ad30445ef26ce0e ("Bluetooth: fix kernel oops in store_pending_adv_report")
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> net/bluetooth/hci_event.c | 73 ++++++++++++++++++++++++++++++---------
> 1 file changed, 56 insertions(+), 17 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index a4c3703f2e94..6925c090a9e0 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -5599,24 +5599,41 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
> static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> {
> u8 num_reports = skb->data[0];
> - void *ptr = &skb->data[1];
>
> hci_dev_lock(hdev);
>
> + skb_pull(skb, sizeof(num_reports));
> +
> while (num_reports--) {
> - struct hci_ev_le_advertising_info *ev = ptr;
> + struct hci_ev_le_advertising_info *ev;
> s8 rssi;
>
> - if (ev->length <= HCI_MAX_AD_LENGTH) {
> - rssi = ev->data[ev->length];
> - process_adv_report(hdev, ev->evt_type, &ev->bdaddr,
> - ev->bdaddr_type, NULL, 0, rssi,
> - ev->data, ev->length, false);
> - } else {
> - bt_dev_err(hdev, "Dropping invalid advertising data");
> + if (skb->len < sizeof(*ev)) {
> + bt_dev_err(hdev, "Malformed advertising report");
> + break;
> + }
> +
> + ev = (void *) skb->data;
> + skb_pull(skb, sizeof(*ev));
> +
> + if (skb->len < ev->length || ev->length > HCI_MAX_AD_LENGTH) {
> + bt_dev_err(hdev, "Malformed advertising data");
> + break;
> }
>
> - ptr += sizeof(*ev) + ev->length + 1;
> + skb_pull(skb, ev->length);
> +
> + if (skb->len < sizeof(rssi)) {
> + bt_dev_err(hdev, "Malformed advertising rssi");
> + break;
> + }
> +
> + rssi = skb->data[0];
> + skb_pull(skb, sizeof(rssi));
> +
> + process_adv_report(hdev, ev->evt_type, &ev->bdaddr,
> + ev->bdaddr_type, NULL, 0, rssi,
> + ev->data, ev->length, false);
> }
>
> hci_dev_unlock(hdev);
> @@ -5669,15 +5686,31 @@ static u8 ext_evt_type_to_legacy(struct hci_dev *hdev, u16 evt_type)
> static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> {
> u8 num_reports = skb->data[0];
> - void *ptr = &skb->data[1];
>
> hci_dev_lock(hdev);
>
> + skb_pull(skb, sizeof(num_reports));
> +
> while (num_reports--) {
> - struct hci_ev_le_ext_adv_report *ev = ptr;
> + struct hci_ev_le_ext_adv_report *ev;
> u8 legacy_evt_type;
> u16 evt_type;
>
> + if (skb->len < sizeof(*ev)) {
> + bt_dev_err(hdev, "Malformed ext advertising report");
> + break;
> + }
> +
> + ev = (void *) skb->data;
> + skb_pull(skb, sizeof(*ev));
> +
> + if (skb->len < ev->length || ev->length > HCI_MAX_AD_LENGTH) {
> + bt_dev_err(hdev, "Malformed ext advertising data");
> + break;
> + }
> +
> + skb_pull(skb, ev->length);
> +
> evt_type = __le16_to_cpu(ev->evt_type);
> legacy_evt_type = ext_evt_type_to_legacy(hdev, evt_type);
> if (legacy_evt_type != LE_ADV_INVALID) {
> @@ -5687,7 +5720,6 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> !(evt_type & LE_EXT_ADV_LEGACY_PDU));
> }
>
> - ptr += sizeof(*ev) + ev->length;
> }
>
> hci_dev_unlock(hdev);
> @@ -5873,19 +5905,26 @@ static void hci_le_direct_adv_report_evt(struct hci_dev *hdev,
> struct sk_buff *skb)
> {
> u8 num_reports = skb->data[0];
> - void *ptr = &skb->data[1];
>
> hci_dev_lock(hdev);
>
> + skb_pull(skb, sizeof(num_reports));
> +
> while (num_reports--) {
> - struct hci_ev_le_direct_adv_info *ev = ptr;
> + struct hci_ev_le_direct_adv_info *ev;
> +
> + if (skb->len < sizeof(*ev)) {
> + bt_dev_err(hdev, "Malformed direct advertising");
> + break;
> + }
> +
> + ev = (void *) skb->data;
> + skb_pull(skb, sizeof(*ev));
>
> process_adv_report(hdev, ev->evt_type, &ev->bdaddr,
> ev->bdaddr_type, &ev->direct_addr,
> ev->direct_addr_type, ev->rssi, NULL, 0,
> false);
> -
> - ptr += sizeof(*ev);
> }
>
> hci_dev_unlock(hdev);
> --
> 2.26.2
Are both these two patches in the series still something which needs
to be applied as fixes, or has it been solved in other ways?
If so can you shorten the Fixes tags to the first 12 characters of the
SHA-1 ID, cf.
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
.
Regards,
Salvatore
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-13 8:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-24 0:22 [PATCH v3 1/2] Bluetooth: Fix not checking advertisement bondaries Luiz Augusto von Dentz
2020-10-24 0:22 ` [PATCH v3 2/2] Bluetooth: A2MP: Fix not setting request ID Luiz Augusto von Dentz
2021-02-13 8:25 ` [PATCH v3 1/2] Bluetooth: Fix not checking advertisement bondaries Salvatore Bonaccorso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).