Messages in this thread | | | Date | Mon, 14 Feb 2022 12:15:02 +0100 | From | Michał Kępień <> | Subject | Re: [PATCH v3 4/4] mtdchar: add MEMREAD ioctl |
| |
Richard,
Thank you for taking a look at this patch series.
> > + if (req.start + req.len > mtd->size) { > > I think this can overflow since both req.start and req.len are u64. > So an evil-doer might bypass this check.
You are right, thanks. I adopted this check from mtd_check_oob_ops() and your comment made me think that maybe the MEMREADOOB64/MEMWRITEOOB64 ioctls are affected as well, but it looks like 'len' is a 32-bit integer in those other cases, so they look safe to me.
However, the MEMWRITE ioctl does seem to be affected by the same issue since commit f6562bca84d22525f792305e3106571f8714d057 ("mtdchar: prevent unbounded allocation in MEMWRITE ioctl"), see mtdchar_write_ioctl().
Changing the 'len' and 'ooblen' fields of struct mtd_{read,write}_req to u32 would break userspace, so that is not an option. Would truncating req.len to 32 bits (req.len &= 0xffffffff) early in the two relevant functions be the way to go? I guess such a change should be reflected in include/uapi/mtd/mtd-abi.h, too.
-- Best regards, Michał Kępień
| |