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

[Gustavo A. R. Silva]

On Wed, May 17, 2023 at 10:50:53AM -0700, 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

Do you mean the one you commented on yesterday?

https://lore.kernel.org/linux-hardening/ZGPk3PpvYzjD1+0%2F@work/ this?

--
Gustavo

> like hfi_cmds.c:73 is an existing sizing bug that is now fixed with this
> patch, yes?
> 
> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
> 
> -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@xxxxxxxxxx>
> > ---
> >  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
> > 
> 
> -- 
> Kees Cook