Date: Sat, 17 Feb 2024 15:18:59 -0600
From: Alex Elder <elder@...e.org>
To: Erick Archer <erick.archer@....com>,
 Vaibhav Agarwal <vaibhav.sr@...il.com>, Mark Greer <mgreer@...malcreek.com>,
 Johan Hovold <johan@...nel.org>, Alex Elder <elder@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Kees Cook <keescook@...omium.org>
Cc: greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
 linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] greybus: audio: apbridgea: Remove flexible array from
 struct audio_apbridgea_hdr

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@....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@...aro.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
> 

Powered by blists - more mailing lists