linux-kernel - Re: Re: [PATCH v2] usb: gadget: uvc: add framebased stream support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 23 Feb 2022 10:06:48 +0800 (GMT+08:00)
From:   "Jing Leng" <3090101217@....edu.cn>
To:     "Greg KH" <gregkh@...uxfoundation.org>
Cc:     balbi@...nel.org, bilbao@...edu, corbet@....net,
        laurent.pinchart@...asonboard.com, mchehab+huawei@...nel.org,
        pawell@...ence.com, rdunlap@...radead.org,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        "Jing Leng" <jleng@...arella.com>
Subject: Re: Re: [PATCH v2] usb: gadget: uvc: add framebased stream support

Hi Greg KH,

> Why not use a union here as this is coming from the hardware, right?
>

I used a union in PATCH v1
(https://patchwork.kernel.org/project/linux-usb/patch/20220216081651.9089-1-3090101217@zju.edu.cn/),
I compiled it to arm64 binary with GCC 11.2.1, the binary works properly.
But "kernel test robot <lkp@...el.com>" reported a warnings:
 >> drivers/usb/gadget/function/uvc_configfs.c:1091:3: warning: 
 field  within 'struct uvcg_frame::(unnamed at drivers/usb/gadget/function/uvc_configfs.c:1068:2)' 
 is less aligned than 'union uvcg_frame::(anonymous at drivers/usb/gadget/function/uvc_configfs.c:1091:3)' 
 and is usually due to 'struct uvcg_frame::(unnamed at drivers/usb/gadget/function/uvc_configfs.c:1068:2)' 
 being packed, which can lead to unaligned accesses [-Wunaligned-access]
                   union {
                   ^
   1 warning generated.
So I use another way to handle the frame structure.

> Why is this writable, but the other variables are not?
> 

1. bFormatIndex is automatic auto calculated by the driver.
   So it is read-only.
2. I don't know why "b_aspect_ratio_x / b_aspect_ratio_y / bm_interface_flags"
   are read-only. Perhaps these parameters can be obtained directly from 
   the actual stream, so driver doesn't need to take care of these parameters.
3. If bVariableSize is 1, then dwBytesPerLine must be setted to zero (0).
   If bVariableSize is 0, then dwBytesPerLine can be setted to other value.
   So it is writable.

> > -		*size += sizeof(frm->frame);
> > +		*size += sizeof(frm->frame) - 4;
> 
> Where did "4" come from?
>

Uncompressed frame doesn't have "u32 dw_bytes_perline".
Framebased frame doesn't have "u32 dw_max_video_frame_buffer_size".
If we use a union like PATCH v1, there's no need to do this.
Maybe we can add "#define UVCG_SUB_FRAME_PAYLOAD_LENGTH 26", and use
"UVCG_SUB_FRAME_PAYLOAD_LENGTH" to replace "sizeof(frm->frame) - 4"
for the new PATCH.

> > +	/* bVariableSize is only for framebased format. */
> > +	__u8  bVariableSize;
> 
> This just changed a user visable structure size.  What broke when doing
> this?  What tool uses this?
> 

As long as users use "UVC_DT_FORMAT_UNCOMPRESSED_SIZE" instead of
"sizeof(struct uvc_format_uncompressed)" to get the length, there is
no problem. So I have the following modifications:
    -			*size += sizeof(u->desc);
    +			*size += u->desc.bLength;

Currently this change does not break the kernel, and uvc stream APP
based on UVC gadget doesn't need to use "struct uvc_format_uncompressed".

There may be some tools which use it, they can use 
"UVC_DT_FORMAT_UNCOMPRESSED_SIZE" to cover the modification.
In addition, we don't need "copy all uncompressed code, rename
uncompressed as framebased, and make a little change" to access framebased
stream support.

Thanks,
Jing Leng