[PATCH] Input: exc3000 - zero-initialize vendor response buffers

[PATCH] Input: exc3000 - zero-initialize vendor response buffers

[Edson Bruno]

exc3000_vendor_data_request() copies up to EXC3000_LEN_FRAME - 1 bytes
of device-supplied data into the caller's buffer and returns the length
reported by the device. The callers (the fw_version, model and type
sysfs attributes, and probe) pass uninitialized stack buffers and do
not check that length.

If the device returns a short reply, fw_version_show() tests and prints
response[2] and response[3] uninitialized. If the reply is not NUL
terminated, the "%s" conversions read past the copied data into
uninitialized stack, which is then exposed via the world-readable
sysfs attributes.

Zero-initialize the buffers. At most EXC3000_LEN_FRAME - 1 bytes are
ever copied, so the last byte stays zero and the strings are always
terminated.

The problem was found while reviewing the driver with an LLM assistant,
which also helped draft this changelog.

Fixes: d862a3068ea5 ("Input: exc3000 - add support to query model and fw_version")
Assisted-by: Claude:claude-sonnet-5-5
Signed-off-by: Edson Bruno <cloudflare.ddos21@gmail.com>
---
 drivers/input/touchscreen/exc3000.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
index 037bb35238a3..40d74f6deeb8 100644
--- a/drivers/input/touchscreen/exc3000.c
+++ b/drivers/input/touchscreen/exc3000.c
@@ -262,7 +262,7 @@ static ssize_t fw_version_show(struct device *dev,
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct exc3000_data *data = i2c_get_clientdata(client);
-	u8 response[EXC3000_LEN_FRAME];
+	u8 response[EXC3000_LEN_FRAME] = {};
 	int ret;
 
 	/* query bootloader info */
@@ -292,7 +292,7 @@ static ssize_t model_show(struct device *dev,
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct exc3000_data *data = i2c_get_clientdata(client);
-	u8 response[EXC3000_LEN_FRAME];
+	u8 response[EXC3000_LEN_FRAME] = {};
 	int ret;
 
 	ret = exc3000_vendor_data_request(data, (u8[]){'E'}, 1, response, 1);
@@ -308,7 +308,7 @@ static ssize_t type_show(struct device *dev,
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct exc3000_data *data = i2c_get_clientdata(client);
-	u8 response[EXC3000_LEN_FRAME];
+	u8 response[EXC3000_LEN_FRAME] = {};
 	int ret;
 
 	ret = exc3000_vendor_data_request(data, (u8[]){'F'}, 1, response, 1);
@@ -410,7 +410,7 @@ static int exc3000_probe(struct i2c_client *client)
 	 * or two touch events anyways).
 	 */
 	for (retry = 0; retry < 3; retry++) {
-		u8 response[EXC3000_LEN_FRAME];
+		u8 response[EXC3000_LEN_FRAME] = {};
 
 		error = exc3000_vendor_data_request(data, (u8[]){'E'}, 1,
 						    response, 1);
-- 
2.53.0

Re: [PATCH] Input: exc3000 - zero-initialize vendor response buffers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Input: exc3000: race on data->buf in exc3000_vendor_data_request()
- [Medium] Input: exc3000 - stale data in fw_version_show() when reusing response buffer
- [Medium] Input: exc3000: off-by-4 bounds check in exc3000_vendor_data_request()
- [Low] Input: exc3000 - Fixes tag references wrong commit for uninitialized buffers
--

commit 390612be535b37b6ded7e34eebf63dafb87943fe
Author: Edson Bruno <cloudflare.ddos21@gmail.com>

Input: exc3000 - zero-initialize vendor response buffers

This commit zero-initializes the stack-allocated vendor response buffers in
fw_version_show(), model_show(), type_show(), and exc3000_probe(). This is
intended to prevent reading uninitialized stack memory when the device returns
a short or non-null-terminated reply to exc3000_vendor_data_request().

>     Input: exc3000 - zero-initialize vendor response buffers
>     
>     exc3000_vendor_data_request() copies up to EXC3000_LEN_FRAME - 1 bytes
>     of device-supplied data into the caller's buffer and returns the length
>     reported by the device. The callers (the fw_version, model and type
>     sysfs attributes, and probe) pass uninitialized stack buffers and do
>     not check that length.
>     
>     If the device returns a short reply, fw_version_show() tests and prints
>     response[2] and response[3] uninitialized. If the reply is not NUL
>     terminated, the "%s" conversions read past the copied data into
>     uninitialized stack, which is then exposed via the world-readable
>     sysfs attributes.
>     
>     Zero-initialize the buffers. At most EXC3000_LEN_FRAME - 1 bytes are
>     ever copied, so the last byte stays zero and the strings are always
>     terminated.
>     
>     The problem was found while reviewing the driver with an LLM assistant,
>     which also helped draft this changelog.
>     
>     Fixes: d862a3068ea5 ("Input: exc3000 - add support to query model and fw_version")
>     Assisted-by: Claude:claude-sonnet-5-5
>     Signed-off-by: Edson Bruno <cloudflare.ddos21@gmail.com>

[Severity: Low]
Does the Fixes tag reference the right commit for the uninitialized stack
buffers in drivers/input/touchscreen/exc3000.c?

Commit d862a3068ea5 ("Input: exc3000 - add support to query model and
fw_version") stored model and fw_version in devm_kzalloc()-allocated struct
exc3000_data fields and populated them via strlcpy(), which guaranteed null
termination and did not use stack response buffers.

The uninitialized stack buffers and queries touched by this patch were
introduced across later commits:
- Commit 102feb1ddfd00d ("Input: exc3000 - factor out vendor data request")
  removed strlcpy() and introduced exc3000_vendor_data_request() along with
  the uninitialized u8 response[EXC3000_LEN_FRAME] stack buffers in
  fw_version_show(), model_show(), and exc3000_probe().
- Commit c929ac9eb85acf ("Input: exc3000 - fix firmware version query for
  device in bootloader") added the bootloader query reading response[2] and
  response[3] in fw_version_show().
- Commit ad117c558e838f ("Input: exc3000 - add type sysfs attribute") added
  type_show() with its uninitialized u8 response[EXC3000_LEN_FRAME] stack
  buffer.

Should the Fixes tags reference those commits instead so stable backport
tooling does not target kernels down to d862a3068ea5 where the patched code
does not exist?

> diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
> index d1f9b44d9d76b..2530b5935fefe 100644
> --- a/drivers/input/touchscreen/exc3000.c
> +++ b/drivers/input/touchscreen/exc3000.c
> @@ -262,7 +262,7 @@ static ssize_t fw_version_show(struct device *dev,
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct exc3000_data *data = i2c_get_clientdata(client);
> -	u8 response[EXC3000_LEN_FRAME];
> +	u8 response[EXC3000_LEN_FRAME] = {};
>  	int ret;
>  
>  	/* query bootloader info */

[Severity: Medium]
In drivers/input/touchscreen/exc3000.c, fw_version_show() zero-initializes
response[EXC3000_LEN_FRAME] once at declaration, but passes response to
exc3000_vendor_data_request() twice when the device is in application mode:

drivers/input/touchscreen/exc3000.c:fw_version_show() {
	u8 response[EXC3000_LEN_FRAME] = {};
	int ret;

	/* query bootloader info */
	ret = exc3000_vendor_data_request(data,
					  (u8[]){0x39, 0x02}, 2, response, 1);
	if (ret < 0)
		return ret;
...
	if (response[2] || response[3])
		return sprintf(buf, "%d.%d\n", response[2], response[3]);

	ret = exc3000_vendor_data_request(data, (u8[]){'D'}, 1, response, 1);
	if (ret < 0)
		return ret;

	return sprintf(buf, "%s\n", &response[1]);
}

In exc3000_vendor_data_request(), memcpy(response, &data->buf[4],
data->buf[3]) only overwrites the first data->buf[3] bytes of response and
does not clear or null-terminate the remainder of the buffer:

drivers/input/touchscreen/exc3000.c:exc3000_vendor_data_request() {
...
	memcpy(response, &data->buf[4], data->buf[3]);
	return data->buf[3];
}

When a user reads the world-readable fw_version sysfs attribute on a device
in application mode, the first exc3000_vendor_data_request() call populates
response[0..ret1-1] (such as response[0] = 0x39 and response[1] = 0x02, with
response[2] == 0 and response[3] == 0), and the second call for (u8[]){'D'}
reuses that dirty response buffer without clearing it.

If the second reply is short (1 byte or less) or shorter than the first reply
and not null-terminated, won't sprintf(buf, "%s\n", &response[1]) read stale
non-zero bytes left behind by the bootloader query?

> @@ -292,7 +292,7 @@ static ssize_t model_show(struct device *dev,
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct exc3000_data *data = i2c_get_clientdata(client);
> -	u8 response[EXC3000_LEN_FRAME];
> +	u8 response[EXC3000_LEN_FRAME] = {};
>  	int ret;
>  
>  	ret = exc3000_vendor_data_request(data, (u8[]){'E'}, 1, response, 1);

[Severity: Medium]
The commit message notes that exc3000_vendor_data_request() copies up to
EXC3000_LEN_FRAME - 1 bytes of device-supplied data into the caller's
response[] buffer in fw_version_show(), model_show(), type_show(), and
exc3000_probe().

However, in drivers/input/touchscreen/exc3000.c, exc3000_interrupt() ->
exc3000_read_frame() reads only a single EXC3000_LEN_FRAME (66) byte frame
into data->buf[0..65]:

drivers/input/touchscreen/exc3000.c:exc3000_read_frame() {
...
	ret = i2c_master_recv(client, buf, EXC3000_LEN_FRAME);
...
}

Because the 4-byte vendor frame header occupies data->buf[0..3] and the
vendor payload starts at &data->buf[4], at most EXC3000_LEN_FRAME - 4 (62)
bytes of vendor payload exist in that received frame:

drivers/input/touchscreen/exc3000.c:exc3000_vendor_data_request() {
...
	if (data->buf[3] >= EXC3000_LEN_FRAME)
		return -ENOSPC;

	memcpy(response, &data->buf[4], data->buf[3]);
	return data->buf[3];
}

Since exc3000_vendor_data_request() checks data->buf[3] >= EXC3000_LEN_FRAME
instead of data->buf[3] > EXC3000_LEN_FRAME - 4, if the device reports
data->buf[3] in 63..65, can memcpy(response, &data->buf[4], data->buf[3])
read up to 3 bytes past the 66-byte vendor frame into data->buf[66..68] (the
second frame slot populated by exc3000_handle_mt_event() when more than 5
touch slots are active) and expose those bytes via the world-readable
fw_version, model, and type sysfs attributes?

> @@ -308,7 +308,7 @@ static ssize_t type_show(struct device *dev,
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct exc3000_data *data = i2c_get_clientdata(client);
> -	u8 response[EXC3000_LEN_FRAME];
> +	u8 response[EXC3000_LEN_FRAME] = {};
>  	int ret;
>  
>  	ret = exc3000_vendor_data_request(data, (u8[]){'F'}, 1, response, 1);

[Severity: High]
In drivers/input/touchscreen/exc3000.c, zero-initializing
response[EXC3000_LEN_FRAME] in fw_version_show(), model_show(), type_show(),
and exc3000_probe() relies on exc3000_vendor_data_request() never copying
more than EXC3000_LEN_FRAME - 1 bytes into response[].

However, exc3000_interrupt() reads frames into the shared data->buf without
acquiring data->query_lock, and returns IRQ_HANDLED immediately after calling
complete(&data->wait_event):

drivers/input/touchscreen/exc3000.c:exc3000_interrupt() {
	ret = exc3000_read_frame(data, buf);
	if (ret) {
		/* Schedule a timer to release "stuck" contacts */
		exc3000_schedule_timer(data);
		goto out;
	}

	switch (buf[2]) {
	case EXC3000_VENDOR_EVENT:
		complete(&data->wait_event);
		break;
...
}

Meanwhile, exc3000_vendor_data_request() checks data->buf[3] and copies from
&data->buf[4] without synchronization against exc3000_interrupt():

drivers/input/touchscreen/exc3000.c:exc3000_vendor_data_request() {
...
	if (data->buf[3] >= EXC3000_LEN_FRAME)
		return -ENOSPC;

	memcpy(response, &data->buf[4], data->buf[3]);
	return data->buf[3];
}

Once exc3000_interrupt() returns IRQ_HANDLED and unmasks the interrupt line,
a subsequent interrupt (such as a touch event EXC3000_MT1_EVENT or
EXC3000_MT2_EVENT, or another device frame) can invoke
exc3000_read_frame(data, data->buf) and overwrite data->buf[0..65] via
i2c_master_recv() while exc3000_vendor_data_request() is concurrently
evaluating data->buf[3] and copying from &data->buf[4].

If data->buf[3] is overwritten between the bounds check and the memcpy()
with a value >= EXC3000_LEN_FRAME, could memcpy(response, &data->buf[4],
data->buf[3]) overflow the caller's 66-byte stack buffer response[] and
overwrite the trailing null terminator?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021192803.776-1-cloudflare.ddos21@gmail.com?part=1