Commit 6420ac0af95d ("mtdchar: prevent unbounded allocation in MEMWRITE
ioctl") added a safety check to mtdchar_write_ioctl() which attempts to
ensure that the write request sent by user space does not extend beyond
the MTD device's size. However, that check contains an addition of two
struct mtd_write_req fields, 'start' and 'len', both of which are u64
variables. The result of that addition can overflow, allowing the
safety check to be bypassed.
The arguably simplest fix - changing the data types of the relevant
struct mtd_write_req fields - is not feasible as it would break user
space.
Fix by making mtdchar_write_ioctl() truncate the value provided by user
space in the 'len' field of struct mtd_write_req, so that only the lower
32 bits of that field are used, preventing the overflow.
While the 'ooblen' field of struct mtd_write_req is not currently used
in any similarly flawed safety check, also truncate it to 32 bits, for
consistency with the 'len' field and with other MTD routines handling
OOB data.
Update include/uapi/mtd/mtd-abi.h accordingly.
Suggested-by: Richard Weinberger <[email protected]>
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/mtd/mtdchar.c | 3 +++
include/uapi/mtd/mtd-abi.h | 4 ++--
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index d0f9c4b0285c..b2700f8467ff 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -615,6 +615,9 @@ static int mtdchar_write_ioctl(struct mtd_info *mtd,
if (!usr_oob)
req.ooblen = 0;
+ req.len &= 0xffffffff;
+ req.ooblen &= 0xffffffff;
+
if (req.start + req.len > mtd->size)
return -EINVAL;
diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
index b869990c2db2..890d9e5b76d7 100644
--- a/include/uapi/mtd/mtd-abi.h
+++ b/include/uapi/mtd/mtd-abi.h
@@ -69,8 +69,8 @@ enum {
* struct mtd_write_req - data structure for requesting a write operation
*
* @start: start address
- * @len: length of data buffer
- * @ooblen: length of OOB buffer
+ * @len: length of data buffer (only lower 32 bits are used)
+ * @ooblen: length of OOB buffer (only lower 32 bits are used)
* @usr_data: user-provided data buffer
* @usr_oob: user-provided OOB buffer
* @mode: MTD mode (see "MTD operation modes")
--
2.36.1
----- Ursprüngliche Mail -----
> Von: "Michał Kępień" <[email protected]>
> An: "Miquel Raynal" <[email protected]>, "richard" <[email protected]>, "Vignesh Raghavendra" <[email protected]>
> CC: "linux-mtd" <[email protected]>, "linux-kernel" <[email protected]>
> Gesendet: Montag, 16. Mai 2022 09:06:00
> Betreff: [PATCH 1/2] mtdchar: prevent integer overflow in a safety check
> Commit 6420ac0af95d ("mtdchar: prevent unbounded allocation in MEMWRITE
> ioctl") added a safety check to mtdchar_write_ioctl() which attempts to
> ensure that the write request sent by user space does not extend beyond
> the MTD device's size. However, that check contains an addition of two
> struct mtd_write_req fields, 'start' and 'len', both of which are u64
> variables. The result of that addition can overflow, allowing the
> safety check to be bypassed.
>
> The arguably simplest fix - changing the data types of the relevant
> struct mtd_write_req fields - is not feasible as it would break user
> space.
>
> Fix by making mtdchar_write_ioctl() truncate the value provided by user
> space in the 'len' field of struct mtd_write_req, so that only the lower
> 32 bits of that field are used, preventing the overflow.
>
> While the 'ooblen' field of struct mtd_write_req is not currently used
> in any similarly flawed safety check, also truncate it to 32 bits, for
> consistency with the 'len' field and with other MTD routines handling
> OOB data.
>
> Update include/uapi/mtd/mtd-abi.h accordingly.
>
> Suggested-by: Richard Weinberger <[email protected]>
> Signed-off-by: Michał Kępień <[email protected]>
> ---
> drivers/mtd/mtdchar.c | 3 +++
> include/uapi/mtd/mtd-abi.h | 4 ++--
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index d0f9c4b0285c..b2700f8467ff 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -615,6 +615,9 @@ static int mtdchar_write_ioctl(struct mtd_info *mtd,
> if (!usr_oob)
> req.ooblen = 0;
>
> + req.len &= 0xffffffff;
> + req.ooblen &= 0xffffffff;
> +
Yeah, I think it is reasonable to limit write requests to 4GiB.
Thanks,
//richard
On Mon, 2022-05-16 at 07:06:00 UTC, =?utf-8?b?TWljaGHFgiBLxJlwaWXFhA==?= wrote:
> Commit 6420ac0af95d ("mtdchar: prevent unbounded allocation in MEMWRITE
> ioctl") added a safety check to mtdchar_write_ioctl() which attempts to
> ensure that the write request sent by user space does not extend beyond
> the MTD device's size. However, that check contains an addition of two
> struct mtd_write_req fields, 'start' and 'len', both of which are u64
> variables. The result of that addition can overflow, allowing the
> safety check to be bypassed.
>
> The arguably simplest fix - changing the data types of the relevant
> struct mtd_write_req fields - is not feasible as it would break user
> space.
>
> Fix by making mtdchar_write_ioctl() truncate the value provided by user
> space in the 'len' field of struct mtd_write_req, so that only the lower
> 32 bits of that field are used, preventing the overflow.
>
> While the 'ooblen' field of struct mtd_write_req is not currently used
> in any similarly flawed safety check, also truncate it to 32 bits, for
> consistency with the 'len' field and with other MTD routines handling
> OOB data.
>
> Update include/uapi/mtd/mtd-abi.h accordingly.
>
> Suggested-by: Richard Weinberger <[email protected]>
> Signed-off-by: Michał Kępień <[email protected]>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.
Miquel