Re: [PATCH] leds: class: Protect brightness_show() with led_cdev->led_access mutex

[anish kumar]

On Tue, Oct 15, 2024 at 9:26 AM Mukesh Ojha <quic_mojha@xxxxxxxxxxx> wrote:
>
> There is NULL pointer issue observed if from Process A where hid device
> being added which results in adding a led_cdev addition and later a
> another call to access of led_cdev attribute from Process B can result
> in NULL pointer issue.

Which pointer is NULL? Call stack shows that dualshock4_led_get_brightness
function could be culprit?

>
> Use mutex led_cdev->led_access to protect access to led->cdev and its
> attribute inside brightness_show().

I don't think it is needed here because it is just calling the led driver
callback and updating the brightness. So, why would we need to serialize
that using mutex? Maybe the callback needs some debugging.
I'm curious if it is ready by the time the callback is invoked.

>
>         Process A                               Process B
>
>  kthread+0x114
>  worker_thread+0x244
>  process_scheduled_works+0x248
>  uhid_device_add_worker+0x24
>  hid_add_device+0x120
>  device_add+0x268
>  bus_probe_device+0x94
>  device_initial_probe+0x14
>  __device_attach+0xfc
>  bus_for_each_drv+0x10c
>  __device_attach_driver+0x14c
>  driver_probe_device+0x3c
>  __driver_probe_device+0xa0
>  really_probe+0x190
>  hid_device_probe+0x130
>  ps_probe+0x990
>  ps_led_register+0x94
>  devm_led_classdev_register_ext+0x58
>  led_classdev_register_ext+0x1f8
>  device_create_with_groups+0x48
>  device_create_groups_vargs+0xc8
>  device_add+0x244
>  kobject_uevent+0x14
>  kobject_uevent_env[jt]+0x224
>  mutex_unlock[jt]+0xc4
>  __mutex_unlock_slowpath+0xd4
>  wake_up_q+0x70
>  try_to_wake_up[jt]+0x48c
>  preempt_schedule_common+0x28
>  __schedule+0x628
>  __switch_to+0x174
>                                                 el0t_64_sync+0x1a8/0x1ac
>                                                 el0t_64_sync_handler+0x68/0xbc
>                                                 el0_svc+0x38/0x68
>                                                 do_el0_svc+0x1c/0x28
>                                                 el0_svc_common+0x80/0xe0
>                                                 invoke_syscall+0x58/0x114
>                                                 __arm64_sys_read+0x1c/0x2c
>                                                 ksys_read+0x78/0xe8
>                                                 vfs_read+0x1e0/0x2c8
>                                                 kernfs_fop_read_iter+0x68/0x1b4
>                                                 seq_read_iter+0x158/0x4ec
>                                                 kernfs_seq_show+0x44/0x54
>                                                 sysfs_kf_seq_show+0xb4/0x130
>                                                 dev_attr_show+0x38/0x74
>                                                 brightness_show+0x20/0x4c
>                                                 dualshock4_led_get_brightness+0xc/0x74
>
> [ 3313.874295][ T4013] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000060
> [ 3313.874301][ T4013] Mem abort info:
> [ 3313.874303][ T4013]   ESR = 0x0000000096000006
> [ 3313.874305][ T4013]   EC = 0x25: DABT (current EL), IL = 32 bits
> [ 3313.874307][ T4013]   SET = 0, FnV = 0
> [ 3313.874309][ T4013]   EA = 0, S1PTW = 0
> [ 3313.874311][ T4013]   FSC = 0x06: level 2 translation fault
> [ 3313.874313][ T4013] Data abort info:
> [ 3313.874314][ T4013]   ISV = 0, ISS = 0x00000006, ISS2 = 0x00000000
> [ 3313.874316][ T4013]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> [ 3313.874318][ T4013]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [ 3313.874320][ T4013] user pgtable: 4k pages, 39-bit VAs, pgdp=00000008f2b0a000
> ..
>
> [ 3313.874332][ T4013] Dumping ftrace buffer:
> [ 3313.874334][ T4013]    (ftrace buffer empty)
> ..
> ..
> [ dd3313.874639][ T4013] CPU: 6 PID: 4013 Comm: InputReader
> [ 3313.874648][ T4013] pc : dualshock4_led_get_brightness+0xc/0x74
> [ 3313.874653][ T4013] lr : led_update_brightness+0x38/0x60
> [ 3313.874656][ T4013] sp : ffffffc0b910bbd0
> ..
> ..
> [ 3313.874685][ T4013] Call trace:
> [ 3313.874687][ T4013]  dualshock4_led_get_brightness+0xc/0x74
> [ 3313.874690][ T4013]  brightness_show+0x20/0x4c
> [ 3313.874692][ T4013]  dev_attr_show+0x38/0x74
> [ 3313.874696][ T4013]  sysfs_kf_seq_show+0xb4/0x130
> [ 3313.874700][ T4013]  kernfs_seq_show+0x44/0x54
> [ 3313.874703][ T4013]  seq_read_iter+0x158/0x4ec
> [ 3313.874705][ T4013]  kernfs_fop_read_iter+0x68/0x1b4
> [ 3313.874708][ T4013]  vfs_read+0x1e0/0x2c8
> [ 3313.874711][ T4013]  ksys_read+0x78/0xe8
> [ 3313.874714][ T4013]  __arm64_sys_read+0x1c/0x2c
> [ 3313.874718][ T4013]  invoke_syscall+0x58/0x114
> [ 3313.874721][ T4013]  el0_svc_common+0x80/0xe0
> [ 3313.874724][ T4013]  do_el0_svc+0x1c/0x28
> [ 3313.874727][ T4013]  el0_svc+0x38/0x68
> [ 3313.874730][ T4013]  el0t_64_sync_handler+0x68/0xbc
> [ 3313.874732][ T4013]  el0t_64_sync+0x1a8/0x1ac
>
> Signed-off-by: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> ---
>  drivers/leds/led-class.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 06b97fd49ad9..e3cb93f19c06 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -30,8 +30,9 @@ static ssize_t brightness_show(struct device *dev,
>  {
>         struct led_classdev *led_cdev = dev_get_drvdata(dev);
>
> -       /* no lock needed for this */

>> also you missed this.

> +       mutex_lock(&led_cdev->led_access);
>         led_update_brightness(led_cdev);
> +       mutex_unlock(&led_cdev->led_access);
>
>         return sprintf(buf, "%u\n", led_cdev->brightness);
>  }
> --
> 2.34.1
>
>