| Message ID | 20210601184616.GA23488@embeddedor |
|---|---|
| State | Accepted |
| Commit | c73c23f347168e315d65fd3b7cffca8439724b26 |
| Headers | show |
| Series | [venus-for-next-v5.14] media: venus: hfi_cmds: Fix packet size calculation | expand |
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 >> >
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
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;
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(-)