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[] = {
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
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
-
Johan Hovold -
Rui Miguel Silva