| Message ID | ZGQn63U4IeRUiJWb@work (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [next] media: venus: hfi_cmds: Replace fake flex-array with flexible-array member | expand |
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@kernel.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? 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 <gustavoars@kernel.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 {
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 <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> > --- > 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 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@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> > > --- > > 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 <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 >> >
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 {
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@kernel.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(-)