* [PATCH][venus-for-next-v5.14] media: venus: hfi_cmds: Fix packet size calculation
@ 2021-06-01 18:46 Gustavo A. R. Silva
2021-06-02 19:55 ` Kees Cook
0 siblings, 1 reply; 4+ messages in thread
From: Gustavo A. R. Silva @ 2021-06-01 18:46 UTC (permalink / raw)
To: Stanimir Varbanov, Andy Gross, Bjorn Andersson,
Mauro Carvalho Chehab
Cc: linux-media, linux-arm-msm, linux-kernel, Gustavo A. R. Silva,
linux-hardening
Now that a one-element array was replaced with a flexible-array member
in struct hfi_sys_set_property_pkt, use the struct_size() helper to
correctly calculate the packet size.
Fixes: 701e10b3fd9f ("media: venus: hfi_cmds.h: Replace one-element array with flexible-array member")
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
BTW... it seems that a similar problem is present in
https://lore.kernel.org/linux-hardening/20210211001044.GA69612@embeddedor/
and that is what is causing the regression. I will send v2 of that
patch, shortly. Thanks.
drivers/media/platform/qcom/venus/hfi_cmds.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c
index 11a8347e5f5c..c86279e5d6e8 100644
--- a/drivers/media/platform/qcom/venus/hfi_cmds.c
+++ b/drivers/media/platform/qcom/venus/hfi_cmds.c
@@ -27,7 +27,7 @@ void pkt_sys_idle_indicator(struct hfi_sys_set_property_pkt *pkt, u32 enable)
{
struct hfi_enable *hfi = (struct hfi_enable *)&pkt->data[1];
- pkt->hdr.size = sizeof(*pkt) + sizeof(*hfi) + sizeof(u32);
+ pkt->hdr.size = struct_size(pkt, data, 2) + sizeof(*hfi);
pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
pkt->num_properties = 1;
pkt->data[0] = HFI_PROPERTY_SYS_IDLE_INDICATOR;
@@ -39,7 +39,7 @@ void pkt_sys_debug_config(struct hfi_sys_set_property_pkt *pkt, u32 mode,
{
struct hfi_debug_config *hfi;
- pkt->hdr.size = sizeof(*pkt) + sizeof(*hfi) + sizeof(u32);
+ pkt->hdr.size = struct_size(pkt, data, 2) + sizeof(*hfi);
pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
pkt->num_properties = 1;
pkt->data[0] = HFI_PROPERTY_SYS_DEBUG_CONFIG;
@@ -50,7 +50,7 @@ void pkt_sys_debug_config(struct hfi_sys_set_property_pkt *pkt, u32 mode,
void pkt_sys_coverage_config(struct hfi_sys_set_property_pkt *pkt, u32 mode)
{
- pkt->hdr.size = sizeof(*pkt) + sizeof(u32);
+ pkt->hdr.size = struct_size(pkt, data, 2);
pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
pkt->num_properties = 1;
pkt->data[0] = HFI_PROPERTY_SYS_CONFIG_COVERAGE;
@@ -116,7 +116,7 @@ void pkt_sys_power_control(struct hfi_sys_set_property_pkt *pkt, u32 enable)
{
struct hfi_enable *hfi = (struct hfi_enable *)&pkt->data[1];
- pkt->hdr.size = sizeof(*pkt) + sizeof(*hfi) + sizeof(u32);
+ pkt->hdr.size = struct_size(pkt, data, 2) + sizeof(*hfi);
pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
pkt->num_properties = 1;
pkt->data[0] = HFI_PROPERTY_SYS_CODEC_POWER_PLANE_CTRL;
--
2.27.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH][venus-for-next-v5.14] media: venus: hfi_cmds: Fix packet size calculation
2021-06-01 18:46 [PATCH][venus-for-next-v5.14] media: venus: hfi_cmds: Fix packet size calculation Gustavo A. R. Silva
@ 2021-06-02 19:55 ` Kees Cook
2021-06-02 20:34 ` Gustavo A. R. Silva
0 siblings, 1 reply; 4+ messages in thread
From: Kees Cook @ 2021-06-02 19:55 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Stanimir Varbanov, Andy Gross, Bjorn Andersson,
Mauro Carvalho Chehab, linux-media, linux-arm-msm, linux-kernel,
linux-hardening
On Tue, Jun 01, 2021 at 01:46:16PM -0500, Gustavo A. R. Silva wrote:
> Now that a one-element array was replaced with a flexible-array member
> in struct hfi_sys_set_property_pkt, use the struct_size() helper to
> correctly calculate the packet size.
>
> Fixes: 701e10b3fd9f ("media: venus: hfi_cmds.h: Replace one-element array with flexible-array member")
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> BTW... it seems that a similar problem is present in
> https://lore.kernel.org/linux-hardening/20210211001044.GA69612@embeddedor/
> and that is what is causing the regression. I will send v2 of that
> patch, shortly. Thanks.
>
> drivers/media/platform/qcom/venus/hfi_cmds.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c
> index 11a8347e5f5c..c86279e5d6e8 100644
> --- a/drivers/media/platform/qcom/venus/hfi_cmds.c
> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c
> @@ -27,7 +27,7 @@ void pkt_sys_idle_indicator(struct hfi_sys_set_property_pkt *pkt, u32 enable)
> {
> struct hfi_enable *hfi = (struct hfi_enable *)&pkt->data[1];
>
> - pkt->hdr.size = sizeof(*pkt) + sizeof(*hfi) + sizeof(u32);
> + pkt->hdr.size = struct_size(pkt, data, 2) + sizeof(*hfi);
I think this should be "1" not "2".
(i.e. there is a single "data" item, followed by an entire *hfi (which
starts immediate after data[0]).
> pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
> pkt->num_properties = 1;
> pkt->data[0] = HFI_PROPERTY_SYS_IDLE_INDICATOR;
> @@ -39,7 +39,7 @@ void pkt_sys_debug_config(struct hfi_sys_set_property_pkt *pkt, u32 mode,
> {
> struct hfi_debug_config *hfi;
>
> - pkt->hdr.size = sizeof(*pkt) + sizeof(*hfi) + sizeof(u32);
> + pkt->hdr.size = struct_size(pkt, data, 2) + sizeof(*hfi);
Same here.
> pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
> pkt->num_properties = 1;
> pkt->data[0] = HFI_PROPERTY_SYS_DEBUG_CONFIG;
> @@ -50,7 +50,7 @@ void pkt_sys_debug_config(struct hfi_sys_set_property_pkt *pkt, u32 mode,
>
> void pkt_sys_coverage_config(struct hfi_sys_set_property_pkt *pkt, u32 mode)
> {
> - pkt->hdr.size = sizeof(*pkt) + sizeof(u32);
> + pkt->hdr.size = struct_size(pkt, data, 2);
This looks correct.
> pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
> pkt->num_properties = 1;
> pkt->data[0] = HFI_PROPERTY_SYS_CONFIG_COVERAGE;
> @@ -116,7 +116,7 @@ void pkt_sys_power_control(struct hfi_sys_set_property_pkt *pkt, u32 enable)
> {
> struct hfi_enable *hfi = (struct hfi_enable *)&pkt->data[1];
>
> - pkt->hdr.size = sizeof(*pkt) + sizeof(*hfi) + sizeof(u32);
> + pkt->hdr.size = struct_size(pkt, data, 2) + sizeof(*hfi);
Also 1.
> pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
> pkt->num_properties = 1;
> pkt->data[0] = HFI_PROPERTY_SYS_CODEC_POWER_PLANE_CTRL;
> --
> 2.27.0
>
--
Kees Cook
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH][venus-for-next-v5.14] media: venus: hfi_cmds: Fix packet size calculation
2021-06-02 19:55 ` Kees Cook
@ 2021-06-02 20:34 ` Gustavo A. R. Silva
2021-06-02 22:02 ` Gustavo A. R. Silva
0 siblings, 1 reply; 4+ messages in thread
From: Gustavo A. R. Silva @ 2021-06-02 20:34 UTC (permalink / raw)
To: Kees Cook, Gustavo A. R. Silva
Cc: Stanimir Varbanov, Andy Gross, Bjorn Andersson,
Mauro Carvalho Chehab, linux-media, linux-arm-msm, linux-kernel,
linux-hardening
On 6/2/21 14:55, Kees Cook wrote:
> On Tue, Jun 01, 2021 at 01:46:16PM -0500, Gustavo A. R. Silva wrote:
>> Now that a one-element array was replaced with a flexible-array member
>> in struct hfi_sys_set_property_pkt, use the struct_size() helper to
>> correctly calculate the packet size.
>>
>> Fixes: 701e10b3fd9f ("media: venus: hfi_cmds.h: Replace one-element array with flexible-array member")
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>> BTW... it seems that a similar problem is present in
>> https://lore.kernel.org/linux-hardening/20210211001044.GA69612@embeddedor/
>> and that is what is causing the regression. I will send v2 of that
>> patch, shortly. Thanks.
>>
>> drivers/media/platform/qcom/venus/hfi_cmds.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c
>> index 11a8347e5f5c..c86279e5d6e8 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_cmds.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c
>> @@ -27,7 +27,7 @@ void pkt_sys_idle_indicator(struct hfi_sys_set_property_pkt *pkt, u32 enable)
>> {
>> struct hfi_enable *hfi = (struct hfi_enable *)&pkt->data[1];
>>
>> - pkt->hdr.size = sizeof(*pkt) + sizeof(*hfi) + sizeof(u32);
>> + pkt->hdr.size = struct_size(pkt, data, 2) + sizeof(*hfi);
>
> I think this should be "1" not "2".
>
> (i.e. there is a single "data" item, followed by an entire *hfi (which
> starts immediate after data[0]).
Yeah; I see your point. Here I just wanted to preserve the exact same size
as the original code, which turns out has a "benign" off-by-one issue.
I'll fix it up and respin.
Thanks!
--
Gustavo
>
>> pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
>> pkt->num_properties = 1;
>> pkt->data[0] = HFI_PROPERTY_SYS_IDLE_INDICATOR;
>> @@ -39,7 +39,7 @@ void pkt_sys_debug_config(struct hfi_sys_set_property_pkt *pkt, u32 mode,
>> {
>> struct hfi_debug_config *hfi;
>>
>> - pkt->hdr.size = sizeof(*pkt) + sizeof(*hfi) + sizeof(u32);
>> + pkt->hdr.size = struct_size(pkt, data, 2) + sizeof(*hfi);
>
> Same here.
>
>> pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
>> pkt->num_properties = 1;
>> pkt->data[0] = HFI_PROPERTY_SYS_DEBUG_CONFIG;
>> @@ -50,7 +50,7 @@ void pkt_sys_debug_config(struct hfi_sys_set_property_pkt *pkt, u32 mode,
>>
>> void pkt_sys_coverage_config(struct hfi_sys_set_property_pkt *pkt, u32 mode)
>> {
>> - pkt->hdr.size = sizeof(*pkt) + sizeof(u32);
>> + pkt->hdr.size = struct_size(pkt, data, 2);
>
> This looks correct.
>
>> pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
>> pkt->num_properties = 1;
>> pkt->data[0] = HFI_PROPERTY_SYS_CONFIG_COVERAGE;
>> @@ -116,7 +116,7 @@ void pkt_sys_power_control(struct hfi_sys_set_property_pkt *pkt, u32 enable)
>> {
>> struct hfi_enable *hfi = (struct hfi_enable *)&pkt->data[1];
>>
>> - pkt->hdr.size = sizeof(*pkt) + sizeof(*hfi) + sizeof(u32);
>> + pkt->hdr.size = struct_size(pkt, data, 2) + sizeof(*hfi);
>
> Also 1.
>
>> pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
>> pkt->num_properties = 1;
>> pkt->data[0] = HFI_PROPERTY_SYS_CODEC_POWER_PLANE_CTRL;
>> --
>> 2.27.0
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH][venus-for-next-v5.14] media: venus: hfi_cmds: Fix packet size calculation
2021-06-02 20:34 ` Gustavo A. R. Silva
@ 2021-06-02 22:02 ` Gustavo A. R. Silva
0 siblings, 0 replies; 4+ messages in thread
From: Gustavo A. R. Silva @ 2021-06-02 22:02 UTC (permalink / raw)
To: Kees Cook, Gustavo A. R. Silva
Cc: Stanimir Varbanov, Andy Gross, Bjorn Andersson,
Mauro Carvalho Chehab, linux-media, linux-arm-msm, linux-kernel,
linux-hardening
On 6/2/21 15:34, Gustavo A. R. Silva wrote:
>
>
> On 6/2/21 14:55, Kees Cook wrote:
>> On Tue, Jun 01, 2021 at 01:46:16PM -0500, Gustavo A. R. Silva wrote:
>>> Now that a one-element array was replaced with a flexible-array member
>>> in struct hfi_sys_set_property_pkt, use the struct_size() helper to
>>> correctly calculate the packet size.
>>>
>>> Fixes: 701e10b3fd9f ("media: venus: hfi_cmds.h: Replace one-element array with flexible-array member")
>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>>> ---
>>> BTW... it seems that a similar problem is present in
>>> https://lore.kernel.org/linux-hardening/20210211001044.GA69612@embeddedor/
>>> and that is what is causing the regression. I will send v2 of that
>>> patch, shortly. Thanks.
>>>
>>> drivers/media/platform/qcom/venus/hfi_cmds.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c
>>> index 11a8347e5f5c..c86279e5d6e8 100644
>>> --- a/drivers/media/platform/qcom/venus/hfi_cmds.c
>>> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c
>>> @@ -27,7 +27,7 @@ void pkt_sys_idle_indicator(struct hfi_sys_set_property_pkt *pkt, u32 enable)
>>> {
>>> struct hfi_enable *hfi = (struct hfi_enable *)&pkt->data[1];
>>>
>>> - pkt->hdr.size = sizeof(*pkt) + sizeof(*hfi) + sizeof(u32);
>>> + pkt->hdr.size = struct_size(pkt, data, 2) + sizeof(*hfi);
>>
>> I think this should be "1" not "2".
>>
>> (i.e. there is a single "data" item, followed by an entire *hfi (which
>> starts immediate after data[0]).
>
> Yeah; I see your point. Here I just wanted to preserve the exact same size
> as the original code, which turns out has a "benign" off-by-one issue.
And the reason why I say it's a "benign" off-by-one (which is actually off-by-four-bytes)
issue is because of this piece of code:
drivers/media/platform/qcom/venus/hfi_venus.c:
864 static int venus_sys_set_idle_message(struct venus_hfi_device *hdev,
865 bool enable)
866 {
867 struct hfi_sys_set_property_pkt *pkt;
868 u8 packet[IFACEQ_VAR_SMALL_PKT_SIZE];
869 int ret;
870
871 if (!enable)
872 return 0;
873
874 pkt = (struct hfi_sys_set_property_pkt *)packet;
875
876 pkt_sys_idle_indicator(pkt, enable);
877
878 ret = venus_iface_cmdq_write(hdev, pkt, false);
879 if (ret)
880 return ret;
881
882 return 0;
883 }
IFACEQ_VAR_SMALL_PKT_SIZE at line 868 is of size 100, which is greater than
sizeof(*pkt) + sizeof(*hfi) + sizeof(u32); in the original code:
drivers/media/platform/qcom/venus/hfi_cmds.c:
26 void pkt_sys_idle_indicator(struct hfi_sys_set_property_pkt *pkt, u32 enable)
27 {
28 struct hfi_enable *hfi = (struct hfi_enable *)&pkt->data[1];
29
30 pkt->hdr.size = sizeof(*pkt) + sizeof(*hfi) + sizeof(u32);
31 pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
32 pkt->num_properties = 1;
33 pkt->data[0] = HFI_PROPERTY_SYS_IDLE_INDICATOR;
34 hfi->enable = enable;
35 }
--
Gustavo
> I'll fix it up and respin.
>
> Thanks!
> --
> Gustavo
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-02 22:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-06-01 18:46 [PATCH][venus-for-next-v5.14] media: venus: hfi_cmds: Fix packet size calculation Gustavo A. R. Silva
2021-06-02 19:55 ` Kees Cook
2021-06-02 20:34 ` Gustavo A. R. Silva
2021-06-02 22:02 ` Gustavo A. R. Silva
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.