[PATCH] monitor: Fix minimum size for variable length events

[PATCH] monitor: Fix minimum size for variable length events

[Szymon Janc]

Those could lead to reading invalid memory if frames were corrupted.
---
 monitor/packet.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/monitor/packet.c b/monitor/packet.c
index ba58d84..56a315b 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -8204,7 +8204,7 @@ static const struct event_data event_table[] = {
 	{ 0x01, "Inquiry Complete",
 				inquiry_complete_evt, 1, true },
 	{ 0x02, "Inquiry Result",
-				inquiry_result_evt, 1, false },
+				inquiry_result_evt, 8, false },
 	{ 0x03, "Connect Complete",
 				conn_complete_evt, 11, true },
 	{ 0x04, "Connect Request",
@@ -8238,7 +8238,7 @@ static const struct event_data event_table[] = {
 	{ 0x12, "Role Change",
 				role_change_evt, 8, true },
 	{ 0x13, "Number of Completed Packets",
-				num_completed_packets_evt, 1, false },
+				num_completed_packets_evt, 5, false },
 	{ 0x14, "Mode Change",
 				mode_change_evt, 6, true },
 	{ 0x15, "Return Link Keys",
@@ -8268,7 +8268,7 @@ static const struct event_data event_table[] = {
 	{ 0x21, "Flow Specification Complete",
 				flow_spec_complete_evt, 22, true },
 	{ 0x22, "Inquiry Result with RSSI",
-				inquiry_result_with_rssi_evt, 1, false },
+				inquiry_result_with_rssi_evt, 8, false },
 	{ 0x23, "Read Remote Extended Features",
 				remote_ext_features_complete_evt, 13, true },
 	{ 0x2c, "Synchronous Connect Complete",
@@ -8278,7 +8278,7 @@ static const struct event_data event_table[] = {
 	{ 0x2e, "Sniff Subrating",
 				sniff_subrating_evt, 11, true },
 	{ 0x2f, "Extended Inquiry Result",
-				ext_inquiry_result_evt, 1, false },
+				ext_inquiry_result_evt, 8, false },
 	{ 0x30, "Encryption Key Refresh Complete",
 				encrypt_key_refresh_complete_evt, 3, true },
 	{ 0x31, "IO Capability Request",
@@ -8322,7 +8322,7 @@ static const struct event_data event_table[] = {
 	{ 0x47, "Flow Specification Modify Complete",
 				flow_spec_modify_complete_evt, 3, true },
 	{ 0x48, "Number of Completed Data Blocks",
-				num_completed_data_blocks_evt, 3, false },
+				num_completed_data_blocks_evt, 9, false },
 	{ 0x49, "AMP Start Test" },
 	{ 0x4a, "AMP Test End" },
 	{ 0x4b, "AMP Receiver Report" },
-- 
1.9.3

Re: [PATCH] monitor: Fix minimum size for variable length events

[Marcel Holtmann]

Hi Szymon,

> Those could lead to reading invalid memory if frames were corrupted.
> ---
> monitor/packet.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/monitor/packet.c b/monitor/packet.c
> index ba58d84..56a315b 100644
> --- a/monitor/packet.c
> +++ b/monitor/packet.c
> @@ -8204,7 +8204,7 @@ static const struct event_data event_table[] = {
> 	{ 0x01, "Inquiry Complete",
> 				inquiry_complete_evt, 1, true },
> 	{ 0x02, "Inquiry Result",
> -				inquiry_result_evt, 1, false },
> +				inquiry_result_evt, 8, false },

these are wrong. That is why fixed size is set to false here. It means that the callback function needs to ensure we do the right checks. If we don't, please with the callback functions.

Regards

Marcel

Re: [PATCH] monitor: Fix minimum size for variable length events

[Szymon Janc]

Hi Marcel,

On Wednesday 04 of February 2015 14:03:25 Marcel Holtmann wrote:
> Hi Szymon,
> 
> > Those could lead to reading invalid memory if frames were corrupted.
> > ---
> > monitor/packet.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/monitor/packet.c b/monitor/packet.c
> > index ba58d84..56a315b 100644
> > --- a/monitor/packet.c
> > +++ b/monitor/packet.c
> > @@ -8204,7 +8204,7 @@ static const struct event_data event_table[] = {
> > 
> > 	{ 0x01, "Inquiry Complete",
> > 	
> > 				inquiry_complete_evt, 1, true },
> > 	
> > 	{ 0x02, "Inquiry Result",
> > 
> > -				inquiry_result_evt, 1, false },
> > +				inquiry_result_evt, 8, false },
> 
> these are wrong. That is why fixed size is set to false here. It means that
> the callback function needs to ensure we do the right checks. If we don't,
> please with the callback functions.

If fixed==false then passed size is minimum required size and all callbacks 
are expecting those.

>From packet_hci_event()

	if (event_data->fixed) {
		if (hdr->plen != event_data->size) {
			print_text(COLOR_ERROR, "invalid packet size");
			packet_hexdump(data, size);
			return;
		}
	} else {
		if (hdr->plen < event_data->size) {
			print_text(COLOR_ERROR, "too short packet");
			packet_hexdump(data, size);
			return;
		}
	}

	event_data->func(data, hdr->plen);


-- 
BR
Szymon Janc

Re: [PATCH] monitor: Fix minimum size for variable length events

[Marcel Holtmann]

Hi Szymon,

>>> Those could lead to reading invalid memory if frames were corrupted.
>>> ---
>>> monitor/packet.c | 10 +++++-----
>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/monitor/packet.c b/monitor/packet.c
>>> index ba58d84..56a315b 100644
>>> --- a/monitor/packet.c
>>> +++ b/monitor/packet.c
>>> @@ -8204,7 +8204,7 @@ static const struct event_data event_table[] = {
>>> 
>>> 	{ 0x01, "Inquiry Complete",
>>> 	
>>> 				inquiry_complete_evt, 1, true },
>>> 	
>>> 	{ 0x02, "Inquiry Result",
>>> 
>>> -				inquiry_result_evt, 1, false },
>>> +				inquiry_result_evt, 8, false },
>> 
>> these are wrong. That is why fixed size is set to false here. It means that
>> the callback function needs to ensure we do the right checks. If we don't,
>> please with the callback functions.
> 
> If fixed==false then passed size is minimum required size and all callbacks 
> are expecting those.
> 
> From packet_hci_event()
> 
> 	if (event_data->fixed) {
> 		if (hdr->plen != event_data->size) {
> 			print_text(COLOR_ERROR, "invalid packet size");
> 			packet_hexdump(data, size);
> 			return;
> 		}
> 	} else {
> 		if (hdr->plen < event_data->size) {
> 			print_text(COLOR_ERROR, "too short packet");
> 			packet_hexdump(data, size);
> 			return;
> 		}
> 	}
> 
> 	event_data->func(data, hdr->plen);

yes exactly. And the minimum size for inquiry results is 1 octet. The num can be actually 0.

Regards

Marcel