| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CABCoZhB97E46NTRq-=JeUCH3V9fc45qC0WpA8qN2y6gxvWmbHA@mail.gmail.com>
Date: Tue, 15 Oct 2024 10:59:12 -0700
From: anish kumar <yesanishhere@...il.com>
To: Mukesh Ojha <quic_mojha@...cinc.com>
Cc: Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>, linux-leds@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] leds: class: Protect brightness_show() with
led_cdev->led_access mutex
On Tue, Oct 15, 2024 at 9:26 AM Mukesh Ojha <quic_mojha@...cinc.com> 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@...cinc.com>
> ---
> 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
>
>
Powered by blists - more mailing lists