| Message ID | 20240217154758.7965-1-erick.archer@gmx.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 699b60ba237def5d83c53fbf29be468154ec37a6 |
| Headers | show |
| Series | greybus: audio: apbridgea: Remove flexible array from struct audio_apbridgea_hdr | expand |
On 2/17/24 9:47 AM, Erick Archer wrote: > When a struct containing a flexible array is included in another struct, > and there is a member after the struct-with-flex-array, there is a > possibility of memory overlap. These cases must be audited [1]. See: > > struct inner { > ... > int flex[]; > }; > > struct outer { > ... > struct inner header; > int overlap; > ... > }; > > This is the scenario for the "struct audio_apbridgea_hdr" structure > that is included in the following "struct audio_apbridgea_*_request" > structures: Yeah this was not a very good way to define these header structures, but I'm glad to hear the flexible array at the end was never used. I don't know why it was there; maybe it's an artifact from some other information that got removed. If the code compiles with your change, it ought to be fine. (It compiles for me.) It would be good for Vaibhav or Mark to comment though, maybe they can provide some context. > > struct audio_apbridgea_set_config_request > struct audio_apbridgea_register_cport_request > struct audio_apbridgea_unregister_cport_request > struct audio_apbridgea_set_tx_data_size_request > struct audio_apbridgea_prepare_tx_request > struct audio_apbridgea_start_tx_request > struct audio_apbridgea_stop_tx_request > struct audio_apbridgea_shutdown_tx_request > struct audio_apbridgea_set_rx_data_size_request > struct audio_apbridgea_prepare_rx_request > struct audio_apbridgea_start_rx_request > struct audio_apbridgea_stop_rx_request > struct audio_apbridgea_shutdown_rx_request > > The pattern is like the one shown below: > > struct audio_apbridgea_hdr { > ... > __u8 data[]; > } __packed; > > struct audio_apbridgea_*_request { > struct audio_apbridgea_hdr hdr; > ... > } __packed; > > In this case, the trailing flexible array can be removed because it is > never used. > > Link: https://github.com/KSPP/linux/issues/202 [1] > Signed-off-by: Erick Archer <erick.archer@gmx.com> > --- > Hi everyone, > > I'm not sure this patch is correct. My concerns are: > > The "struct audio_apbridgea_hdr" structure is used as a first member in > all the "struct audio_apbridgea_*_request" structures. And these last > structures are used in the "gb_audio_apbridgea_*(...)" functions. These > functions fill the "request" structure and always use: > > gb_hd_output(connection->hd, &req, sizeof(req), > GB_APB_REQUEST_AUDIO_CONTROL, true); > > Then, the "gb_hd_output(struct gb_host_device *hd, ...)" function calls: > > hd->driver->output(hd, req, size, cmd, async); > > The first parameter to this function is of type: > > struct gb_host_device { > ... > const struct gb_hd_driver *driver; > ... > }; > > And the "gb_hd_driver" structure is defined as: > > struct gb_hd_driver { > ... > int (*output)(struct gb_host_device *hd, void *req, u16 size, u8 cmd, > bool async); > }; > > Therefore, my question is: > Where is the "output" function pointer set? I think I'm missing something. I think it will be drivers/greybus/es2.c:output(). But in any case, the output function will know nothing about the structure of the request, so I think it's unrelated to the change you're proposing. Johan can confirm this. I'd like to hear from these others, but otherwise this change looks good to me. Reviewed-by: Alex Elder <elder@linaro.org> > > I have search for another greybus drivers and I have found that, for > example, the "es2_ap_driver" (drivers/greybus/es2.c) sets the "output" > member in: > > static struct gb_hd_driver es2_driver = { > ... > .output = output, > }; > > I think that the flexible array that I have removed should be used in > the function assigned to the "output" function pointer. But I am not > able to find this function. > > A bit of light on this would be greatly appreciated. > > Thanks, > Erick > --- > drivers/staging/greybus/audio_apbridgea.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/staging/greybus/audio_apbridgea.h b/drivers/staging/greybus/audio_apbridgea.h > index efec0f815efd..ab707d310129 100644 > --- a/drivers/staging/greybus/audio_apbridgea.h > +++ b/drivers/staging/greybus/audio_apbridgea.h > @@ -65,7 +65,6 @@ > struct audio_apbridgea_hdr { > __u8 type; > __le16 i2s_port; > - __u8 data[]; > } __packed; > > struct audio_apbridgea_set_config_request { > -- > 2.25.1 >
On Sat, Feb 17, 2024 at 03:18:59PM -0600, Alex Elder wrote: > On 2/17/24 9:47 AM, Erick Archer wrote: > > When a struct containing a flexible array is included in another struct, > > and there is a member after the struct-with-flex-array, there is a > > possibility of memory overlap. These cases must be audited [1]. See: > > > > struct inner { > > ... > > int flex[]; > > }; > > > > struct outer { > > ... > > struct inner header; > > int overlap; > > ... > > }; > > > > This is the scenario for the "struct audio_apbridgea_hdr" structure > > that is included in the following "struct audio_apbridgea_*_request" > > structures: > > Yeah this was not a very good way to define these header > structures, but I'm glad to hear the flexible array at the > end was never used. I don't know why it was there; maybe > it's an artifact from some other information that got removed. > > If the code compiles with your change, it ought to be fine. > (It compiles for me.) > > It would be good for Vaibhav or Mark to comment though, maybe > they can provide some context. Sorry for the delay guys. The way this was done comes from associated firmware that ran on the APBridge. This goes back a while but I think the packet headers may have been in flux at the time and this was a convenient way to change all of the packets if & when it changed. Anyway, it doesn't seem so convenient now. :) So, yeah, getting rid of it sounds like a good thing to do to me. > I'd like to hear from these others, but otherwise this change > looks good to me. > > Reviewed-by: Alex Elder <elder@linaro.org> > > diff --git a/drivers/staging/greybus/audio_apbridgea.h b/drivers/staging/greybus/audio_apbridgea.h > > index efec0f815efd..ab707d310129 100644 > > --- a/drivers/staging/greybus/audio_apbridgea.h > > +++ b/drivers/staging/greybus/audio_apbridgea.h > > @@ -65,7 +65,6 @@ > > struct audio_apbridgea_hdr { > > __u8 type; > > __le16 i2s_port; > > - __u8 data[]; > > } __packed; > > > > struct audio_apbridgea_set_config_request { > > -- > > 2.25.1 Acked-by: Mark Greer <mgreer@animalcreek.com>
diff --git a/drivers/staging/greybus/audio_apbridgea.h b/drivers/staging/greybus/audio_apbridgea.h index efec0f815efd..ab707d310129 100644 --- a/drivers/staging/greybus/audio_apbridgea.h +++ b/drivers/staging/greybus/audio_apbridgea.h @@ -65,7 +65,6 @@ struct audio_apbridgea_hdr { __u8 type; __le16 i2s_port; - __u8 data[]; } __packed; struct audio_apbridgea_set_config_request {
When a struct containing a flexible array is included in another struct, and there is a member after the struct-with-flex-array, there is a possibility of memory overlap. These cases must be audited [1]. See: struct inner { ... int flex[]; }; struct outer { ... struct inner header; int overlap; ... }; This is the scenario for the "struct audio_apbridgea_hdr" structure that is included in the following "struct audio_apbridgea_*_request" structures: struct audio_apbridgea_set_config_request struct audio_apbridgea_register_cport_request struct audio_apbridgea_unregister_cport_request struct audio_apbridgea_set_tx_data_size_request struct audio_apbridgea_prepare_tx_request struct audio_apbridgea_start_tx_request struct audio_apbridgea_stop_tx_request struct audio_apbridgea_shutdown_tx_request struct audio_apbridgea_set_rx_data_size_request struct audio_apbridgea_prepare_rx_request struct audio_apbridgea_start_rx_request struct audio_apbridgea_stop_rx_request struct audio_apbridgea_shutdown_rx_request The pattern is like the one shown below: struct audio_apbridgea_hdr { ... __u8 data[]; } __packed; struct audio_apbridgea_*_request { struct audio_apbridgea_hdr hdr; ... } __packed; In this case, the trailing flexible array can be removed because it is never used. Link: https://github.com/KSPP/linux/issues/202 [1] Signed-off-by: Erick Archer <erick.archer@gmx.com> --- Hi everyone, I'm not sure this patch is correct. My concerns are: The "struct audio_apbridgea_hdr" structure is used as a first member in all the "struct audio_apbridgea_*_request" structures. And these last structures are used in the "gb_audio_apbridgea_*(...)" functions. These functions fill the "request" structure and always use: gb_hd_output(connection->hd, &req, sizeof(req), GB_APB_REQUEST_AUDIO_CONTROL, true); Then, the "gb_hd_output(struct gb_host_device *hd, ...)" function calls: hd->driver->output(hd, req, size, cmd, async); The first parameter to this function is of type: struct gb_host_device { ... const struct gb_hd_driver *driver; ... }; And the "gb_hd_driver" structure is defined as: struct gb_hd_driver { ... int (*output)(struct gb_host_device *hd, void *req, u16 size, u8 cmd, bool async); }; Therefore, my question is: Where is the "output" function pointer set? I think I'm missing something. I have search for another greybus drivers and I have found that, for example, the "es2_ap_driver" (drivers/greybus/es2.c) sets the "output" member in: static struct gb_hd_driver es2_driver = { ... .output = output, }; I think that the flexible array that I have removed should be used in the function assigned to the "output" function pointer. But I am not able to find this function. A bit of light on this would be greatly appreciated. Thanks, Erick --- drivers/staging/greybus/audio_apbridgea.h | 1 - 1 file changed, 1 deletion(-) -- 2.25.1