linux-kernel - Re: [PATCH 3/3] Input: ims-pcu - Check record size in ims_pcu_flash

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202505281611.A024D45E@keescook>
Date: Wed, 28 May 2025 16:26:18 -0700
From: Kees Cook <kees@...nel.org>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Javier Carrasco <javier.carrasco.cruz@...il.com>,
	David Lechner <dlechner@...libre.com>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Erick Archer <erick.archer@...look.com>,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] Input: ims-pcu - Check record size in
 ims_pcu_flash_firmware()

On Wed, May 28, 2025 at 11:22:24PM +0300, Dan Carpenter wrote:
> The "len" variable comes from the firmware and we generally do
> trust firmware, but it's always better to double check.  If the "len"
> is too large it could result in memory corruption when we do
> "memcpy(fragment->data, rec->data, len);"
> 
> Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@...aro.org>
> ---
> Kees, this is a __counted_by() thing.  Would the checkers catch this?
> We know the maximum valid length for "fragment" is and so it's maybe
> possible to know that "fragment->len = len;" is too long?

I see:

pcu->cmd_buf as:

        u8 cmd_buf[IMS_PCU_BUF_SIZE];

and fragment is:

struct ims_pcu_flash_fmt {
        __le32 addr;
        u8 len;
        u8 data[] __counted_by(len);
};

I assume you're asking about this line:

		fragment->len = len;

I'm not aware of any compiler instrumentation that would bounds check
this -- it was designed to trust these sort of explicit assignments.

This is hardly the only place in the kernel doing this kind of
deserialization into a flexible array structure, so maybe there should
be some kind of helper to do the bounds checking and set the
"counted_by" counter?

#define gimme(from, into, counter, len)				\
	({							\
		int __gimme_rc = -EINVAL			\
		size_t __gimme_size = __member_size(from);	\
		if (__gimme_size >= sizeof(*into) &&		\
		    __gimme_size - sizeof(*into) >= len) {	\
			into = (void *)from;			\
			into->counter = len;			\
			__gimme_rc = 0;				\
		}						\
		__gimme_rc;					\
	})

	rc = gimme(&pcu->cmd_buf[1], fragment, len, len);
	if (rc) {
		dev_err(pcu->dev,
			"Invalid record length in firmware: %d\n", len);
		return rc;
	}

-Kees

> 
>  drivers/input/misc/ims-pcu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index d9ee14b1f451..4581f1c53644 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -844,6 +844,12 @@ static int ims_pcu_flash_firmware(struct ims_pcu *pcu,
>  		addr = be32_to_cpu(rec->addr) / 2;
>  		len = be16_to_cpu(rec->len);
>  
> +		if (len > sizeof(pcu->cmd_buf) - 1 - sizeof(*fragment)) {
> +			dev_err(pcu->dev,
> +				"Invalid record length in firmware: %d\n", len);
> +			return -EINVAL;
> +		}
> +
>  		fragment = (void *)&pcu->cmd_buf[1];
>  		put_unaligned_le32(addr, &fragment->addr);
>  		fragment->len = len;
> -- 
> 2.47.2
> 

-- 
Kees Cook