[PATCH_v3 1/3] android: Fix opcode parameter type from uint16_t to uint8_t

[PATCH_v3 1/3] android: Fix opcode parameter type from uint16_t to uint8_t

[Ravi kumar Veeramally]

---
 android/hal-a2dp.c      | 2 +-
 android/hal-bluetooth.c | 2 +-
 android/hal-hidhost.c   | 2 +-
 android/hal.h           | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/android/hal-a2dp.c b/android/hal-a2dp.c
index f0c301e..4fe7c20 100644
--- a/android/hal-a2dp.c
+++ b/android/hal-a2dp.c
@@ -49,7 +49,7 @@ static void handle_audio_state(void *buf)
 }
 
 /* will be called from notification thread context */
-void bt_notify_a2dp(uint16_t opcode, void *buf, uint16_t len)
+void bt_notify_a2dp(uint8_t opcode, void *buf, uint16_t len)
 {
 	if (!interface_ready())
 		return;
diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
index 3e5d41f..1cfd994 100644
--- a/android/hal-bluetooth.c
+++ b/android/hal-bluetooth.c
@@ -281,7 +281,7 @@ static void handle_acl_state_changed(void *buf)
 }
 
 /* will be called from notification thread context */
-void bt_notify_adapter(uint16_t opcode, void *buf, uint16_t len)
+void bt_notify_adapter(uint8_t opcode, void *buf, uint16_t len)
 {
 	if (!interface_ready())
 		return;
diff --git a/android/hal-hidhost.c b/android/hal-hidhost.c
index 97a1aa0..2ce17a3 100644
--- a/android/hal-hidhost.c
+++ b/android/hal-hidhost.c
@@ -88,7 +88,7 @@ static void handle_virtual_unplug(void *buf)
 }
 
 /* will be called from notification thread context */
-void bt_notify_hidhost(uint16_t opcode, void *buf, uint16_t len)
+void bt_notify_hidhost(uint8_t opcode, void *buf, uint16_t len)
 {
 	if (!interface_ready())
 		return;
diff --git a/android/hal.h b/android/hal.h
index 2ce7932..baa4754 100644
--- a/android/hal.h
+++ b/android/hal.h
@@ -26,8 +26,8 @@ bthh_interface_t *bt_get_hidhost_interface(void);
 btpan_interface_t *bt_get_pan_interface(void);
 btav_interface_t *bt_get_a2dp_interface(void);
 
-void bt_notify_adapter(uint16_t opcode, void *buf, uint16_t len);
+void bt_notify_adapter(uint8_t opcode, void *buf, uint16_t len);
 void bt_thread_associate(void);
 void bt_thread_disassociate(void);
-void bt_notify_hidhost(uint16_t opcode, void *buf, uint16_t len);
-void bt_notify_a2dp(uint16_t opcode, void *buf, uint16_t len);
+void bt_notify_hidhost(uint8_t opcode, void *buf, uint16_t len);
+void bt_notify_a2dp(uint8_t opcode, void *buf, uint16_t len);
-- 
1.8.3.2

[PATCH_v3 2/3] android/pan: Add notify method to PAN notifications

[Ravi kumar Veeramally]

---
 android/hal-pan.c | 6 ++++++
 android/hal.h     | 1 +
 2 files changed, 7 insertions(+)

diff --git a/android/hal-pan.c b/android/hal-pan.c
index bec179f..851c5d2 100644
--- a/android/hal-pan.c
+++ b/android/hal-pan.c
@@ -31,6 +31,12 @@ static bool interface_ready(void)
 	return cbs != NULL;
 }
 
+void bt_notify_pan(uint8_t opcode, void *buf, uint16_t len)
+{
+	if (!interface_ready())
+		return;
+}
+
 static bt_status_t pan_enable(int local_role)
 {
 	struct hal_cmd_pan_enable cmd;
diff --git a/android/hal.h b/android/hal.h
index baa4754..72090fe 100644
--- a/android/hal.h
+++ b/android/hal.h
@@ -31,3 +31,4 @@ void bt_thread_associate(void);
 void bt_thread_disassociate(void);
 void bt_notify_hidhost(uint8_t opcode, void *buf, uint16_t len);
 void bt_notify_a2dp(uint8_t opcode, void *buf, uint16_t len);
+void bt_notify_pan(uint8_t opcode, void *buf, uint16_t len);
-- 
1.8.3.2

[PATCH_v3 3/3] android/pan: Handle connection and control state notifications

[Ravi kumar Veeramally]

---
 android/hal-pan.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/android/hal-pan.c b/android/hal-pan.c
index 851c5d2..2bc560e 100644
--- a/android/hal-pan.c
+++ b/android/hal-pan.c
@@ -31,10 +31,41 @@ static bool interface_ready(void)
 	return cbs != NULL;
 }
 
+static void handle_conn_state(void *buf)
+{
+	struct hal_ev_pan_conn_state *ev = buf;
+
+	if (cbs->connection_state_cb)
+		cbs->connection_state_cb(ev->state, ev->status,
+					(bt_bdaddr_t *) ev->bdaddr,
+					ev->local_role, ev->remote_role);
+}
+
+static void handle_ctrl_state(void *buf)
+{
+	struct hal_ev_pan_ctrl_state *ev = buf;
+
+	if (cbs->control_state_cb)
+		cbs->control_state_cb(ev->state, ev->status,
+					ev->local_role, (char *)ev->name);
+}
+
 void bt_notify_pan(uint8_t opcode, void *buf, uint16_t len)
 {
 	if (!interface_ready())
 		return;
+
+	switch (opcode) {
+	case HAL_EV_PAN_CONN_STATE:
+		handle_conn_state(buf);
+		break;
+	case HAL_EV_PAN_CTRL_STATE:
+		handle_ctrl_state(buf);
+		break;
+	default:
+		DBG("Unhandled callback opcode=0x%x", opcode);
+		break;
+	}
 }
 
 static bt_status_t pan_enable(int local_role)
-- 
1.8.3.2

Re: [PATCH_v3 3/3] android/pan: Handle connection and control state notifications

[Johan Hedberg]

Hi Ravi,

On Wed, Nov 13, 2013, Ravi kumar Veeramally wrote:
> ---
>  android/hal-pan.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)

I've applied the first two patches, but for this one I've got a few
questions:

> diff --git a/android/hal-pan.c b/android/hal-pan.c
> index 851c5d2..2bc560e 100644
> --- a/android/hal-pan.c
> +++ b/android/hal-pan.c
> @@ -31,10 +31,41 @@ static bool interface_ready(void)
>  	return cbs != NULL;
>  }
>  
> +static void handle_conn_state(void *buf)
> +{
> +	struct hal_ev_pan_conn_state *ev = buf;
> +
> +	if (cbs->connection_state_cb)
> +		cbs->connection_state_cb(ev->state, ev->status,
> +					(bt_bdaddr_t *) ev->bdaddr,
> +					ev->local_role, ev->remote_role);
> +}
> +
> +static void handle_ctrl_state(void *buf)
> +{
> +	struct hal_ev_pan_ctrl_state *ev = buf;
> +
> +	if (cbs->control_state_cb)
> +		cbs->control_state_cb(ev->state, ev->status,
> +					ev->local_role, (char *)ev->name);
> +}
> +
>  void bt_notify_pan(uint8_t opcode, void *buf, uint16_t len)
>  {
>  	if (!interface_ready())
>  		return;
> +
> +	switch (opcode) {
> +	case HAL_EV_PAN_CONN_STATE:
> +		handle_conn_state(buf);
> +		break;
> +	case HAL_EV_PAN_CTRL_STATE:
> +		handle_ctrl_state(buf);
> +		break;
> +	default:
> +		DBG("Unhandled callback opcode=0x%x", opcode);
> +		break;
> +	}
>  }

What I don't like about this is that you're not pushing the data length
to the handler functions. If you did that the handler functions could:

	if (len < sizeof(*ev))
		return;

Instead of return we could also just abort - what's the general policy
on the HAL side regarding invalid data from the daemon? How does this
relate to the work Szymon is doing to add proper checks for the IPC
data? Is that only for the daemon side?

Are we missing similar checks in other HALs too?

Johan

Re: [PATCH_v3 3/3] android/pan: Handle connection and control state notifications

[Ravi Kumar Veeramally]

Hi Johan,

On 11/13/2013 11:33 AM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Wed, Nov 13, 2013, Ravi kumar Veeramally wrote:
>> ---
>>   android/hal-pan.c | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
> I've applied the first two patches, but for this one I've got a few
> questions:
>
>> diff --git a/android/hal-pan.c b/android/hal-pan.c
>> index 851c5d2..2bc560e 100644
>> --- a/android/hal-pan.c
>> +++ b/android/hal-pan.c
>> @@ -31,10 +31,41 @@ static bool interface_ready(void)
>>   	return cbs != NULL;
>>   }
>>   
>> +static void handle_conn_state(void *buf)
>> +{
>> +	struct hal_ev_pan_conn_state *ev = buf;
>> +
>> +	if (cbs->connection_state_cb)
>> +		cbs->connection_state_cb(ev->state, ev->status,
>> +					(bt_bdaddr_t *) ev->bdaddr,
>> +					ev->local_role, ev->remote_role);
>> +}
>> +
>> +static void handle_ctrl_state(void *buf)
>> +{
>> +	struct hal_ev_pan_ctrl_state *ev = buf;
>> +
>> +	if (cbs->control_state_cb)
>> +		cbs->control_state_cb(ev->state, ev->status,
>> +					ev->local_role, (char *)ev->name);
>> +}
>> +
>>   void bt_notify_pan(uint8_t opcode, void *buf, uint16_t len)
>>   {
>>   	if (!interface_ready())
>>   		return;
>> +
>> +	switch (opcode) {
>> +	case HAL_EV_PAN_CONN_STATE:
>> +		handle_conn_state(buf);
>> +		break;
>> +	case HAL_EV_PAN_CTRL_STATE:
>> +		handle_ctrl_state(buf);
>> +		break;
>> +	default:
>> +		DBG("Unhandled callback opcode=0x%x", opcode);
>> +		break;
>> +	}
>>   }
> What I don't like about this is that you're not pushing the data length
> to the handler functions. If you did that the handler functions could:
>
> 	if (len < sizeof(*ev))
> 		return;
>
> Instead of return we could also just abort - what's the general policy
> on the HAL side regarding invalid data from the daemon? How does this
> relate to the work Szymon is doing to add proper checks for the IPC
> data? Is that only for the daemon side?
>
> Are we missing similar checks in other HALs too?

   Yes, we are not doing similar checks in other HALs 
(hal-bluetooth/a2dp/hid/) too.
   Very few places in hal-bluetooth length is passing but validation is 
not done.
   I will fix them all and send you the patch.

  Thanks,
  Ravi.

Re: [PATCH_v3 3/3] android/pan: Handle connection and control state notifications

[Szymon Janc]

Hi,

> >> +
> >> +	switch (opcode) {
> >> +	case HAL_EV_PAN_CONN_STATE:
> >> +		handle_conn_state(buf);
> >> +		break;
> >> +	case HAL_EV_PAN_CTRL_STATE:
> >> +		handle_ctrl_state(buf);
> >> +		break;
> >> +	default:
> >> +		DBG("Unhandled callback opcode=0x%x", opcode);
> >> +		break;
> >> +	}
> >>   }
> > What I don't like about this is that you're not pushing the data length
> > to the handler functions. If you did that the handler functions could:
> >
> > 	if (len < sizeof(*ev))
> > 		return;
> >
> > Instead of return we could also just abort - what's the general policy
> > on the HAL side regarding invalid data from the daemon? How does this
> > relate to the work Szymon is doing to add proper checks for the IPC
> > data? Is that only for the daemon side?
> >
> > Are we missing similar checks in other HALs too?
> 
>    Yes, we are not doing similar checks in other HALs 
> (hal-bluetooth/a2dp/hid/) too.
>    Very few places in hal-bluetooth length is passing but validation is 
> not done.
>    I will fix them all and send you the patch.

As mentioned in my RFC, messages have very similar format and can be checked
in single place using some common macros. I'm now working on addressing Johan
comments in that RFC. As suggested I'm going to use tables of handlers instead
switch-case and this should also allow to refactor current code to avoid
switch-cases we currently have. Services will simply register own tables of
handlers for service they implement. Idea is that check will be done in common
place and then handlers will have form of  void handle_foo(struct foo *ev) with
guarantee that passed command/event is memory size valid.

Plan is to make this generic code use both by hal and daemon.

So, I'm not sure if it makes much sense to fix passing buf+len to all handlers
we have now.

If you have other ideas for this please comment.


-- 
BR
Szymon Janc

Re: [PATCH_v3 3/3] android/pan: Handle connection and control state notifications

[Ravi Kumar Veeramally]

On 11/13/2013 11:59 AM, Szymon Janc wrote:
> Hi,
>
>>>> +
>>>> +	switch (opcode) {
>>>> +	case HAL_EV_PAN_CONN_STATE:
>>>> +		handle_conn_state(buf);
>>>> +		break;
>>>> +	case HAL_EV_PAN_CTRL_STATE:
>>>> +		handle_ctrl_state(buf);
>>>> +		break;
>>>> +	default:
>>>> +		DBG("Unhandled callback opcode=0x%x", opcode);
>>>> +		break;
>>>> +	}
>>>>    }
>>> What I don't like about this is that you're not pushing the data length
>>> to the handler functions. If you did that the handler functions could:
>>>
>>> 	if (len < sizeof(*ev))
>>> 		return;
>>>
>>> Instead of return we could also just abort - what's the general policy
>>> on the HAL side regarding invalid data from the daemon? How does this
>>> relate to the work Szymon is doing to add proper checks for the IPC
>>> data? Is that only for the daemon side?
>>>
>>> Are we missing similar checks in other HALs too?
>>     Yes, we are not doing similar checks in other HALs
>> (hal-bluetooth/a2dp/hid/) too.
>>     Very few places in hal-bluetooth length is passing but validation is
>> not done.
>>     I will fix them all and send you the patch.
> As mentioned in my RFC, messages have very similar format and can be checked
> in single place using some common macros. I'm now working on addressing Johan
> comments in that RFC. As suggested I'm going to use tables of handlers instead
> switch-case and this should also allow to refactor current code to avoid
> switch-cases we currently have. Services will simply register own tables of
> handlers for service they implement. Idea is that check will be done in common
> place and then handlers will have form of  void handle_foo(struct foo *ev) with
> guarantee that passed command/event is memory size valid.
>
> Plan is to make this generic code use both by hal and daemon.
>
> So, I'm not sure if it makes much sense to fix passing buf+len to all handlers
> we have now.
>
> If you have other ideas for this please comment.
>
   Ok, good to know.
  Johan: Can you now apply my left over patch?

  Thanks,
  Ravi.

Re: [PATCH_v3 3/3] android/pan: Handle connection and control state notifications

[Johan Hedberg]

Hi Ravi,

On Wed, Nov 13, 2013, Ravi Kumar Veeramally wrote:
> On 11/13/2013 11:59 AM, Szymon Janc wrote:
> >Hi,
> >
> >>>>+
> >>>>+	switch (opcode) {
> >>>>+	case HAL_EV_PAN_CONN_STATE:
> >>>>+		handle_conn_state(buf);
> >>>>+		break;
> >>>>+	case HAL_EV_PAN_CTRL_STATE:
> >>>>+		handle_ctrl_state(buf);
> >>>>+		break;
> >>>>+	default:
> >>>>+		DBG("Unhandled callback opcode=0x%x", opcode);
> >>>>+		break;
> >>>>+	}
> >>>>   }
> >>>What I don't like about this is that you're not pushing the data length
> >>>to the handler functions. If you did that the handler functions could:
> >>>
> >>>	if (len < sizeof(*ev))
> >>>		return;
> >>>
> >>>Instead of return we could also just abort - what's the general policy
> >>>on the HAL side regarding invalid data from the daemon? How does this
> >>>relate to the work Szymon is doing to add proper checks for the IPC
> >>>data? Is that only for the daemon side?
> >>>
> >>>Are we missing similar checks in other HALs too?
> >>    Yes, we are not doing similar checks in other HALs
> >>(hal-bluetooth/a2dp/hid/) too.
> >>    Very few places in hal-bluetooth length is passing but validation is
> >>not done.
> >>    I will fix them all and send you the patch.
> >As mentioned in my RFC, messages have very similar format and can be checked
> >in single place using some common macros. I'm now working on addressing Johan
> >comments in that RFC. As suggested I'm going to use tables of handlers instead
> >switch-case and this should also allow to refactor current code to avoid
> >switch-cases we currently have. Services will simply register own tables of
> >handlers for service they implement. Idea is that check will be done in common
> >place and then handlers will have form of  void handle_foo(struct foo *ev) with
> >guarantee that passed command/event is memory size valid.
> >
> >Plan is to make this generic code use both by hal and daemon.
> >
> >So, I'm not sure if it makes much sense to fix passing buf+len to all handlers
> >we have now.
> >
> >If you have other ideas for this please comment.
> >
>   Ok, good to know.
>  Johan: Can you now apply my left over patch?

Yes, it's already done, however I'd really like to see the patches to
fix all this asap.

Johan