Re: [PATCH v3 4/4] mtdchar: add MEMREAD ioctl

[Michał Kępień]

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ń