All of lore.kernel.org
 help / color / mirror / Atom feed
* [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	[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.