Re: [PATCH 3/3] Input: ims-pcu - Check record size in ims_pcu_flash_firmware()

[Dan Carpenter <dan.carpenter@linaro.org>]

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