Date: Sat, 17 Feb 2024 16:47:58 +0100
From: Erick Archer <erick.archer@....com>
To: 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: Erick Archer <erick.archer@....com>,
	greybus-dev@...ts.linaro.org,
	linux-staging@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: [PATCH] greybus: audio: apbridgea: Remove flexible array from struct audio_apbridgea_hdr

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@....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(-)

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