| 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: <42430248-5904-4571-8e7b-307f739acda9@quicinc.com>
Date: Mon, 30 Jun 2025 12:06:24 +0530
From: AKASH KUMAR <quic_akakum@...cinc.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>, Felipe Balbi <balbi@...nel.org>,
Jack Pham <quic_jackp@...cinc.com>, <kernel@...cinc.com>,
Wesley Cheng
<quic_wcheng@...cinc.com>,
Laurent Pinchart
<laurent.pinchart@...asonboard.com>,
Daniel Scally
<dan.scally@...asonboard.com>,
Vijayavardhan Vennapusa
<quic_vvreddy@...cinc.com>,
Krishna Kurapati <quic_kriskura@...cinc.com>,
<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: gadget: uvc: Initialize color matching descriptors
for frame-based
Hi Greg,
Thanks for reviewing.
On 6/28/2025 8:29 PM, Greg Kroah-Hartman wrote:
> On Wed, Jun 25, 2025 at 03:46:39PM +0530, Akash Kumar wrote:
>> Fix NULL pointer crash in uvcg_framebased_make due to uninitialize
>> color matching descriptor for frame-based format.
>>
>> [ 2.771141][ T486] pc : __uvcg_fill_strm+0x198/0x2cc
>> [ 2.771145][ T486] lr : __uvcg_iter_strm_cls+0xc8/0x17c
>> [ 2.771146][ T486] sp : ffffffc08140bbb0
>> [ 2.771146][ T486] x29: ffffffc08140bbb0 x28: ffffff803bc81380 x27: ffffff8023bbd250
>> [ 2.771147][ T486] x26: ffffff8023bbd250 x25: ffffff803c361348 x24: ffffff803d8e6768
>> [ 2.771148][ T486] x23: 0000000000000004 x22: 0000000000000003 x21: ffffffc08140bc48
>> [ 2.771149][ T486] x20: 0000000000000000 x19: ffffffc08140bc48 x18: ffffffe9f8cf4a00
>> [ 2.771150][ T486] x17: 000000001bf64ec3 x16: 000000001bf64ec3 x15: ffffff8023bbd250
>> [ 2.771151][ T486] x14: 000000000000000f x13: 004c4b40000f4240 x12: 000a2c2a00051615
>> [ 2.771152][ T486] x11: 000000000000004f x10: ffffffe9f76b40ec x9 : ffffffe9f7e389d0
>> [ 2.771153][ T486] x8 : ffffff803d0d31ce x7 : 000f4240000a2c2a x6 : 0005161500028b0a
>> [ 2.771154][ T486] x5 : ffffff803d0d31ce x4 : 0000000000000003 x3 : 0000000000000000
>> [ 2.771155][ T486] x2 : ffffffc08140bc50 x1 : ffffffc08140bc48 x0 : 0000000000000000
>> [ 2.771156][ T486] Call trace:
>> [ 2.771157][ T486] __uvcg_fill_strm+0x198/0x2cc
>> [ 2.771157][ T486] __uvcg_iter_strm_cls+0xc8/0x17c
>> [ 2.771158][ T486] uvcg_streaming_class_allow_link+0x240/0x290
>> [ 2.771159][ T486] configfs_symlink+0x1f8/0x630
>> [ 2.771161][ T486] vfs_symlink+0x114/0x1a0
>> [ 2.771163][ T486] do_symlinkat+0x94/0x28c
>> [ 2.771164][ T486] __arm64_sys_symlinkat+0x54/0x70
>> [ 2.771164][ T486] invoke_syscall+0x58/0x114
>> [ 2.771166][ T486] el0_svc_common+0x80/0xe0
>> [ 2.771168][ T486] do_el0_svc+0x1c/0x28
>> [ 2.771169][ T486] el0_svc+0x3c/0x70
>> [ 2.771172][ T486] el0t_64_sync_handler+0x68/0xbc
>> [ 2.771173][ T486] el0t_64_sync+0x1a8/0x1ac
> What is "[ T486]"?
>
> And where did the beginning of the crash report go?
Will share full crash log, its a thread id and must not belong to a
change commit text, will remove and update message.
>
>> Initialize color matching descriptor for frame-based format to prevent
>> NULL pointer crash.
>> This fix prevents a NULL pointer crash in uvcg_framebased_make due to
>> an uninitialized color matching descriptor.
> What causes an unitialized color matching descriptor to happen? Do we
> have that in the kernel today? Or is this userspace controlled?
> Hardware controlled?
Issue is seen when userspace configuration (via configfs) does not
explicitly define the color matching descriptor.
If color_matching is not found, config_group_find_item() returns NULL.
The code then jumps to out_put_cm, where it calls
config_item_put(color_matching);.
If color_matching is NULL, this will dereference a null pointer, leading
to a crash.
refer: [RESEND v3 6/6] usb: gadget: uvc: Allow creating new color
matching descriptors - Daniel Scally
<https://lore.kernel.org/all/20230202114142.300858-7-dan.scally@ideasonboard.com/>
This is userspace controlled and associated with uvc formats.
I will send a fix to handle from kernel driver as well.
>> Signed-off-by: Akash Kumar <quic_akakum@...cinc.com>
> What git id does this fix?
Will update git id [PATCH v3] usb: gadget: uvc: configfs: Add
frame-based frame format support - Akash Kumar
<https://lore.kernel.org/all/20240927152138.31416-1-quic_akakum@quicinc.com/>
>
>> ---
>> drivers/usb/gadget/function/uvc_configfs.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
>> index f131943254a4..a4a2d3dcb0d6 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>> @@ -2916,8 +2916,15 @@ static struct config_group *uvcg_framebased_make(struct config_group *group,
>> 'H', '2', '6', '4', 0x00, 0x00, 0x10, 0x00,
>> 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71
>> };
>> + struct uvcg_color_matching *color_match;
>> + struct config_item *streaming;
>> struct uvcg_framebased *h;
>>
>> + streaming = group->cg_item.ci_parent;
>> + color_match = uvcg_format_get_default_color_match(streaming);
>> + if (!color_match)
>> + return ERR_PTR(-EINVAL);
>> +
>> h = kzalloc(sizeof(*h), GFP_KERNEL);
>> if (!h)
>> return ERR_PTR(-ENOMEM);
>> @@ -2936,6 +2943,9 @@ static struct config_group *uvcg_framebased_make(struct config_group *group,
>>
>> INIT_LIST_HEAD(&h->fmt.frames);
>> h->fmt.type = UVCG_FRAMEBASED;
>> +
>> + h->fmt.color_matching = color_match;
>> + color_match->refcnt++;
> reference counts are almost never done "by hand" like this, are you sure
> this is right? I don't see the lock being held that is used when
> reading/writing this value elsewhere in the driver, why is this safe
> here?
>
> And shouldn't the changelog text be something like "mirror what we do in
> the uncompressed mode?" Or the other modes? Why is this one the only
> one that does not have this check in it today, was it just forgotten or
> was it intentional?
yes it is not safe, i simply mirrored the code used in other formats,
due you to suggest updating locks or using
refcount_inc.
Thanks,
Akash
Powered by blists - more mailing lists