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: <aD2EZRX8CVuvqjsN@stanley.mountain>
Date: Mon, 2 Jun 2025 14:00:53 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Kees Cook <kees@...nel.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 04:26:18PM -0700, Kees Cook wrote:
> 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;
> 	}

I don't think that really scales...  I don't know how KASAN works
internally.  I was thinking it might track the buffer size when we
assign "fragment = (void *)&pcu->cmd_buf[1];" so it could calculate
the valid values of ->len.  But that's actually quite complicated.

Smatch does track this:

drivers/input/misc/ims-pcu.c:856 ims_pcu_flash_firmware() buf size: 'fragment->data' 119 elements, 119 bytes

But:

1) Smatch doesn't know about __counted_by().  This is just a matter of
   writing the code in Sparse.
2) It's not treating fw->data[] as user controlled data because this
   driver loads the firmware asynchronously and Smatch gets confused by
   threads.

regards,
dan carpenter