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

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, 17 May 2023 19:18:01 +0200
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Stanimir Varbanov <stanimir.k.varbanov@...il.com>,
        Vikash Garodia <quic_vgarodia@...cinc.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>
Cc:     linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] media: venus: hfi_cmds: Replace fake flex-array
 with flexible-array member



On 17.05.2023 04:11, Konrad Dybcio wrote:
> 
> 
> On 17.05.2023 03:03, 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.
>>
>> 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@...nel.org>
>> ---
>>  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[];
> Would making this an u32* be a better resolution?
Nevermind, I overthought this by thinking in the terms of its size
and not the data within the struct...

Maybe struct_size could be used instead of subtracting sizeof(u32)
though?

Konrad
> 
> Konrad
>>  };
>>  
>>  struct hfi_sys_release_resource_pkt {