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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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?
Konrad
> };
>
> struct hfi_sys_release_resource_pkt {
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 <[email protected]>
>> ---
>> 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 {
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 <[email protected]>
>
> -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 <[email protected]>
> > ---
> > 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
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?
Reviewed-by: Kees Cook <[email protected]>
-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 <[email protected]>
> ---
> 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
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 <[email protected]>
>
> -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 <[email protected]>
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
>>
>