[PATCH] staging: greybus: timesync: validate platform state callback

[PATCH] staging: greybus: timesync: validate platform state callback

[Rui Miguel Silva]

When tearingdown timesync, and not in arche platform, the state platform
callback is not initialized. That will trigger the following NULL
dereferencing.
CallTrace:

 ? gb_timesync_platform_unlock_bus+0x11/0x20 [greybus]
 gb_timesync_teardown+0x85/0xc0 [greybus]
 gb_timesync_svc_remove+0xab/0x190 [greybus]
 gb_svc_del+0x29/0x110 [greybus]
 gb_hd_del+0x14/0x20 [greybus]
 ap_disconnect+0x24/0x60 [gb_es2]
 usb_unbind_interface+0x7a/0x2c0
 __device_release_driver+0x96/0x150
 device_release_driver+0x1e/0x30
 bus_remove_device+0xe7/0x130
 device_del+0x116/0x230
 usb_disable_device+0x97/0x1f0
 usb_disconnect+0x80/0x260
 hub_event+0x5ca/0x10e0
 process_one_work+0x126/0x3b0
 worker_thread+0x55/0x4c0
 ? process_one_work+0x3b0/0x3b0
 kthread+0xc4/0xe0
 ? kthread_park+0xb0/0xb0
 ret_from_fork+0x22/0x30

So, fix that by adding checks before use the callback.

Cc: <stable@vger.kernel.org> # 4.9.x
Signed-off-by: Rui Miguel Silva <rmfrfs@gmail.com>
---
Greg,
This patch does not contain the "commit <sha> upstream" snippet since you
already removed the timesync code from the -next tree. So, It would never get
there. However this will affect the greybus users of 4.9.x.

 drivers/staging/greybus/timesync_platform.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/timesync_platform.c b/drivers/staging/greybus/timesync_platform.c
index 113f3d6c4b3a..7d3ccc2bc98c 100644
--- a/drivers/staging/greybus/timesync_platform.c
+++ b/drivers/staging/greybus/timesync_platform.c
@@ -45,13 +45,18 @@ u32 gb_timesync_platform_get_clock_rate(void)
 
 int gb_timesync_platform_lock_bus(struct gb_timesync_svc *pdata)
 {
+	if (!arche_platform_change_state_cb)
+		return 0;
+
 	return arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_TIME_SYNC,
 					      pdata);
 }
 
 void gb_timesync_platform_unlock_bus(void)
 {
-	arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_ACTIVE, NULL);
+	if (arche_platform_change_state_cb)
+		arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_ACTIVE,
+					       NULL);
 }
 
 static const struct of_device_id arch_timer_of_match[] = {
-- 
2.11.0

Re: [PATCH] staging: greybus: timesync: validate platform state callback

[Johan Hovold]

On Mon, Jan 23, 2017 at 02:22:44PM +0000, Rui Miguel Silva wrote:
> When tearingdown timesync, and not in arche platform, the state platform
> callback is not initialized. That will trigger the following NULL
> dereferencing.
> CallTrace:
> 
>  ? gb_timesync_platform_unlock_bus+0x11/0x20 [greybus]
>  gb_timesync_teardown+0x85/0xc0 [greybus]
>  gb_timesync_svc_remove+0xab/0x190 [greybus]
>  gb_svc_del+0x29/0x110 [greybus]
>  gb_hd_del+0x14/0x20 [greybus]
>  ap_disconnect+0x24/0x60 [gb_es2]
>  usb_unbind_interface+0x7a/0x2c0
>  __device_release_driver+0x96/0x150
>  device_release_driver+0x1e/0x30
>  bus_remove_device+0xe7/0x130
>  device_del+0x116/0x230
>  usb_disable_device+0x97/0x1f0
>  usb_disconnect+0x80/0x260
>  hub_event+0x5ca/0x10e0
>  process_one_work+0x126/0x3b0
>  worker_thread+0x55/0x4c0
>  ? process_one_work+0x3b0/0x3b0
>  kthread+0xc4/0xe0
>  ? kthread_park+0xb0/0xb0
>  ret_from_fork+0x22/0x30
> 
> So, fix that by adding checks before use the callback.
> 
> Cc: <stable@vger.kernel.org> # 4.9.x
> Signed-off-by: Rui Miguel Silva <rmfrfs@gmail.com>
> ---
> Greg,
> This patch does not contain the "commit <sha> upstream" snippet since you
> already removed the timesync code from the -next tree. So, It would never get
> there. However this will affect the greybus users of 4.9.x.

This code is still in Linus tree and 4.10-rc so this is just a normal
fix even if this is no longer an issue in -next.

>  drivers/staging/greybus/timesync_platform.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/timesync_platform.c b/drivers/staging/greybus/timesync_platform.c
> index 113f3d6c4b3a..7d3ccc2bc98c 100644
> --- a/drivers/staging/greybus/timesync_platform.c
> +++ b/drivers/staging/greybus/timesync_platform.c
> @@ -45,13 +45,18 @@ u32 gb_timesync_platform_get_clock_rate(void)
>  
>  int gb_timesync_platform_lock_bus(struct gb_timesync_svc *pdata)
>  {
> +	if (!arche_platform_change_state_cb)
> +		return 0;
> +
>  	return arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_TIME_SYNC,
>  					      pdata);
>  }
>  
>  void gb_timesync_platform_unlock_bus(void)
>  {
> -	arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_ACTIVE, NULL);
> +	if (arche_platform_change_state_cb)
> +		arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_ACTIVE,
> +					       NULL);

I'd use the same pattern here:

	if (!arche_platform_change_state_cb)
		return;

	arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_ACTIVE, NULL);

Thanks,
Johan

Re: [PATCH] staging: greybus: timesync: validate platform state callback

[Rui Miguel Silva]

Hi Johan,
On Mon, Jan 23, 2017 at 04:28:08PM +0100, Johan Hovold wrote:
>On Mon, Jan 23, 2017 at 02:22:44PM +0000, Rui Miguel Silva wrote:
>> When tearingdown timesync, and not in arche platform, the state platform
>> callback is not initialized. That will trigger the following NULL
>> dereferencing.
>> CallTrace:
>>
>>  ? gb_timesync_platform_unlock_bus+0x11/0x20 [greybus]
>>  gb_timesync_teardown+0x85/0xc0 [greybus]
>>  gb_timesync_svc_remove+0xab/0x190 [greybus]
>>  gb_svc_del+0x29/0x110 [greybus]
>>  gb_hd_del+0x14/0x20 [greybus]
>>  ap_disconnect+0x24/0x60 [gb_es2]
>>  usb_unbind_interface+0x7a/0x2c0
>>  __device_release_driver+0x96/0x150
>>  device_release_driver+0x1e/0x30
>>  bus_remove_device+0xe7/0x130
>>  device_del+0x116/0x230
>>  usb_disable_device+0x97/0x1f0
>>  usb_disconnect+0x80/0x260
>>  hub_event+0x5ca/0x10e0
>>  process_one_work+0x126/0x3b0
>>  worker_thread+0x55/0x4c0
>>  ? process_one_work+0x3b0/0x3b0
>>  kthread+0xc4/0xe0
>>  ? kthread_park+0xb0/0xb0
>>  ret_from_fork+0x22/0x30
>>
>> So, fix that by adding checks before use the callback.
>>
>> Cc: <stable@vger.kernel.org> # 4.9.x
>> Signed-off-by: Rui Miguel Silva <rmfrfs@gmail.com>
>> ---
>> Greg,
>> This patch does not contain the "commit <sha> upstream" snippet since you
>> already removed the timesync code from the -next tree. So, It would never get
>> there. However this will affect the greybus users of 4.9.x.
>
>This code is still in Linus tree and 4.10-rc so this is just a normal
>fix even if this is no longer an issue in -next.
>

Yeah, but this code is not in staging-next anymore, It was mainly an
headsup for that, this patch will not apply to that tree.

>
>>  drivers/staging/greybus/timesync_platform.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/greybus/timesync_platform.c b/drivers/staging/greybus/timesync_platform.c
>> index 113f3d6c4b3a..7d3ccc2bc98c 100644
>> --- a/drivers/staging/greybus/timesync_platform.c
>> +++ b/drivers/staging/greybus/timesync_platform.c
>> @@ -45,13 +45,18 @@ u32 gb_timesync_platform_get_clock_rate(void)
>>
>>  int gb_timesync_platform_lock_bus(struct gb_timesync_svc *pdata)
>>  {
>> +	if (!arche_platform_change_state_cb)
>> +		return 0;
>> +
>>  	return arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_TIME_SYNC,
>>  					      pdata);
>>  }
>>
>>  void gb_timesync_platform_unlock_bus(void)
>>  {
>> -	arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_ACTIVE, NULL);
>> +	if (arche_platform_change_state_cb)
>> +		arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_ACTIVE,
>> +					       NULL);
>
>I'd use the same pattern here:
>
>	if (!arche_platform_change_state_cb)
>		return;
>
>	arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_ACTIVE, NULL);
>

I also notice I forgot the Fixes: tag, so I will add your suggestion
to my v2.

Thanks Johan,
Cheers,
   Rui