Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
To: "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Cc: Andy Nguyen <theflow@google.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Balakrishna Godavarthi <bgodavar@codeaurora.org>,
	Alain Michaud <alainm@chromium.org>,
	Sonny Sasaka <sonnysasaka@chromium.org>,
	Marcel Holtmann <marcel@holtmann.org>
Subject: Re: [PATCH 1/2] Bluetooth: Fix not checking advertisement bondaries
Date: Fri, 16 Oct 2020 11:35:24 -0700
Message-ID: <CABBYNZ+ABX=rVAtopUoWc5rFKMcPeooek+OqjESuB4oUzao8FA@mail.gmail.com> (raw)
In-Reply-To: <20201016180956.707681-1-luiz.dentz@gmail.com>

Hi,

Im CC you guys since you appear in the original fix, if you can please
tests the following patch with the malformed advertisement or let me
know how you were able to generate it I would appreciate it.

On Fri, Oct 16, 2020 at 11:09 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> 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 | 72 ++++++++++++++++++++++++++++++---------
>  1 file changed, 55 insertions(+), 17 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index a4c3703f2e94..ae31d227730b 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -5599,24 +5599,40 @@ 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 = get_unaligned(skb->data);
> +
> +               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 +5685,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 +5719,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 +5904,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
>


-- 
Luiz Augusto von Dentz

      parent reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-16 18:09 Luiz Augusto von Dentz
2020-10-16 18:09 ` [PATCH 2/2] Bluetooth: A2MP: Fix not setting request ID Luiz Augusto von Dentz
2020-10-16 18:35 ` Luiz Augusto von Dentz [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABBYNZ+ABX=rVAtopUoWc5rFKMcPeooek+OqjESuB4oUzao8FA@mail.gmail.com' \
    --to=luiz.dentz@gmail.com \
    --cc=alainm@chromium.org \
    --cc=bgodavar@codeaurora.org \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=marcel@holtmann.org \
    --cc=sonnysasaka@chromium.org \
    --cc=theflow@google.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org
	public-inbox-index linux-bluetooth

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git