| Message ID | 131fd1ae92c828ee9f4fa2de03d8c210ae1f3524.1748463049.git.dan.carpenter@linaro.org |
|---|---|
| State | Accepted |
| Commit | a95ef0199e80f3384eb992889322957d26c00102 |
| Headers | show |
| Series | ihex: add some bounds checking to firmware parsing | expand |
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> Applied, thank you.
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;
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? drivers/input/misc/ims-pcu.c | 6 ++++++ 1 file changed, 6 insertions(+)