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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 14 Feb 2022 12:15:02 +0100
From:   Michał Kępień <kernel@...pniu.pl>
To:     Richard Weinberger <richard@....at>
Cc:     Miquel Raynal <miquel.raynal@...tlin.com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Boris Brezillon <boris.brezillon@...labora.com>,
        linux-mtd <linux-mtd@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
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ń