Re: [PATCH][next] media: venus: hfi_cmds: Replace fake flex-array with flexible-array member

[Vikash Garodia <quic_vgarodia@quicinc.com>]

On 5/17/2023 11:20 PM, Kees Cook wrote:
> On Tue, May 16, 2023 at 07:03:39PM -0600, Gustavo A. R. Silva wrote:
>> One-element arrays are deprecated, and we are replacing them with flexible
>> array members instead. So, replace one-element arrays with flexible-array
>> members in struct hfi_sys_set_resource_pkt, and refactor the rest of
>> the code, accordingly.
>>
>> This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
>> routines on memcpy() and help us make progress towards globally
>> enabling -fstrict-flex-arrays=3 [1].
>>
>> The only binary differences seen before/after changes are the
>> following:
>>
>>      17ba:      mov    %rbx,%rdi
>>      17bd:      call   17c2 <pkt_sys_set_resource+0x42>
>>                         17be: R_X86_64_PLT32    __tsan_write4-0x4
>> -    17c2:      movl   $0x14,(%rbx)
>> +    17c2:      movl   $0x10,(%rbx)
>>      17c8:      lea    0x4(%rbx),%rdi
>>      17cc:      call   17d1 <pkt_sys_set_resource+0x51>
>>                         17cd: R_X86_64_PLT32    __tsan_write4-0x4
>>
>> which is expected once this accounts for the following line of code
>> at  drivers/media/platform/qcom/venus/hfi_cmds.c:73
>>
>> 73         pkt->hdr.size = sizeof(*pkt);
>>
>> and as *pkt is of type struct hfi_sys_set_resource_pkt, sizeof(*pkt) is
>> reduced by 4 bytes, due to the flex-array transformation.
> 
> Based on the other place that was subtracting the 1 element, this looks
> like hfi_cmds.c:73 is an existing sizing bug that is now fixed with this
> patch, yes?

It is not a bug. The way it is structured is like [packet header + payload]. The
packet header size should be (size of header + payload). Line 73 calculates the
size of packet header, while line 86 calculates size of payload. Since payload
starts from the last u32, the total size needs to be reduced by size of u32.

> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> -Kees
> 
>>
>> Link: https://github.com/KSPP/linux/issues/79
>> Link: https://github.com/KSPP/linux/issues/293
>> Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

The patch looks good. As stated in previous patch, lets combine this into a
single series.

>> ---
>>  drivers/media/platform/qcom/venus/hfi_cmds.c | 2 +-
>>  drivers/media/platform/qcom/venus/hfi_cmds.h | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c
>> index 3f74d518ad08..7c82e212434e 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_cmds.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c
>> @@ -83,7 +83,7 @@ int pkt_sys_set_resource(struct hfi_sys_set_resource_pkt *pkt, u32 id, u32 size,
>>  		res->size = size;
>>  		res->mem = addr;
>>  		pkt->resource_type = HFI_RESOURCE_OCMEM;
>> -		pkt->hdr.size += sizeof(*res) - sizeof(u32);
>> +		pkt->hdr.size += sizeof(*res);
>>  		break;
>>  	}
>>  	case VIDC_RESOURCE_NONE:
>> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.h b/drivers/media/platform/qcom/venus/hfi_cmds.h
>> index ba74d03eb9cd..dd9c5066442d 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_cmds.h
>> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.h
>> @@ -56,7 +56,7 @@ struct hfi_sys_set_resource_pkt {
>>  	struct hfi_pkt_hdr hdr;
>>  	u32 resource_handle;
>>  	u32 resource_type;
>> -	u32 resource_data[1];
>> +	u32 resource_data[];
>>  };
>>  
>>  struct hfi_sys_release_resource_pkt {
>> -- 
>> 2.34.1
>>
>