Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

sorry for the multiple emails but I have checked again the code and
looks like process_adv_report() reads from ev->data for a size of
ev->length.

I attach a patch that applies the bound check to both
hci_le_ext_adv_report_evt() and hci_le_adv_report_evt().

Let me know your opinion,
Best regards,
Tomas

On 3/30/19 10:20 AM, Tomas Bortoli wrote:
> Hi Dan,
> 
> On 3/30/19 8:25 AM, Dan Carpenter wrote:
>> There is a potential out of bounds if "ev->length" is too high or if the
>> number of reports are too many.
>>
>> Fixes: c215e9397b00 ("Bluetooth: Process extended ADV report event")
>> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Reviewed-By: Tomas Bortoli <tomasbortoli@xxxxxxxxx>
> 
>> ---
>> Not tested.  I suck at pointer math, and I don't know why the protocol
>> requires a "+ 1".  Please review carefully.
>>
>>  net/bluetooth/hci_event.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 609fd6871c5a..ee945b3d12e1 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -5417,6 +5417,7 @@ 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];
>> +	void *end = &skb->data[skb->len];
>>  
>>  	hci_dev_lock(hdev);
>>  
>> @@ -5425,6 +5426,8 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
>>  		u8 legacy_evt_type;
>>  		u16 evt_type;
>>  
>> +		if (ptr + sizeof(*ev) + ev->length + 1 > end)
>> +			break;
> 
> Assuming that per each iteration, the while cycle, including the call to
> process_adv_report() read up to (sizeof(*ev) + ev->length + 1) bytes,
> 
> (I don't understand why the +1, but, anyway..)
> 
> If the assumption is correct, then the if condition should be:
> 
> if (ptr + sizeof(*ev) + ev->length + 1 >= end)
> 
> Because as soon as we try to read from the `end` pointer on, we are
> out-of-bound.. the valid skb bytes end at `end-1` (included)
> 
> Note that also hci_le_adv_report_evt() is likely to need the same fix!
> 
> 
>>  		evt_type = __le16_to_cpu(ev->evt_type);
>>  		legacy_evt_type = ext_evt_type_to_legacy(evt_type);
>>  		if (legacy_evt_type != LE_ADV_INVALID) {
>>
> 
> 
> Kind regards,
> Tomas
> 

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 609fd6871c5a..275926e0753e 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -5345,6 +5345,7 @@ 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];
+	u8 *end = &skb->data[skb->len - 1];
 
 	hci_dev_lock(hdev);
 
@@ -5352,6 +5353,9 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		struct hci_ev_le_advertising_info *ev = ptr;
 		s8 rssi;
 
+		if (ev->data + ev->length > end)
+			break;
+
 		if (ev->length <= HCI_MAX_AD_LENGTH) {
 			rssi = ev->data[ev->length];
 			process_adv_report(hdev, ev->evt_type, &ev->bdaddr,
@@ -5417,6 +5421,7 @@ 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];
+	u8 *end = &skb->data[skb->len - 1];
 
 	hci_dev_lock(hdev);
 
@@ -5425,6 +5430,9 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		u8 legacy_evt_type;
 		u16 evt_type;
 
+		if (ev->data + ev->length > end)
+			break;
+
 		evt_type = __le16_to_cpu(ev->evt_type);
 		legacy_evt_type = ext_evt_type_to_legacy(evt_type);
 		if (legacy_evt_type != LE_ADV_INVALID) {

[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux