From patchwork Mon Oct 25 08:21:04 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?TWljaGHFgiBLxJlwaWXFhA==?= X-Patchwork-Id: 1545611 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: bilbo.ozlabs.org; dkim=pass (2048-bit key; secure) header.d=lists.infradead.org header.i=@lists.infradead.org header.a=rsa-sha256 header.s=bombadil.20210309 header.b=nV21+GEr; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=kempniu.pl header.i=@kempniu.pl header.a=rsa-sha256 header.s=google header.b=Kc2Wfddg; dkim-atps=neutral Authentication-Results: ozlabs.org; spf=none (no SPF record) smtp.mailfrom=lists.infradead.org (client-ip=2607:7c80:54:e::133; helo=bombadil.infradead.org; envelope-from=linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org; receiver=) Received: from bombadil.infradead.org (bombadil.infradead.org [IPv6:2607:7c80:54:e::133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by bilbo.ozlabs.org (Postfix) with ESMTPS id 4Hd7H20Xk7z9t0k for ; Mon, 25 Oct 2021 19:22:22 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-Id:Date:Subject:Cc :To:From:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References: List-Owner; bh=WAAETPaUOdnqrJgmcqmz7w+z5fOosGhdqAFy4KGntZw=; b=nV21+GErLS4B6S eyF7LTCL9tTPjaeNsZ/P2Yg3ana7QTpZ2HiR/ihfAdqpMQGGU1FiGBcPYzXqb/SibDXF9PPvKBa7e HC0aP6mSYoESBUVPiCYWKtLRd18uAQyp3NME3X5Bmjn+jsUchubE/f7WGCNRrvaulSosBtdK62LyE vkqU4qyCAocxEPPdDHjNduc+DjVWQk+xrKuXBABGk9y5U2Xd+/N0cMCQlNoxDDTfreWt0aPEfPZhb iyMfB+1YwExyYR7xjWZWbdaZQwpPwIUne5eoUlvMJ3zNg/hrLmUUsgfo+M5XFDeAtFsrDJmcxUjeV D5bFDDwdnbAf4TwHmTdQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mevEh-00FmOJ-Ke; Mon, 25 Oct 2021 08:21:39 +0000 Received: from mail-lf1-x135.google.com ([2a00:1450:4864:20::135]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mevEd-00FmLV-GY for linux-mtd@lists.infradead.org; Mon, 25 Oct 2021 08:21:37 +0000 Received: by mail-lf1-x135.google.com with SMTP id p16so10741387lfa.2 for ; Mon, 25 Oct 2021 01:21:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kempniu.pl; s=google; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=J1eGsGvSupt9keTxPYnbOs6SIupFozzDdx1fHvMuh2A=; b=Kc2WfddgrbdOM5M6itU0cenw9rjcU6/6XnRDC8Zwv7sQi9ycnqZhBft49kEbo9YykX Ral/IUibv2TT6eBr721BhIi/oUar/vSxay6ecQmMZ2pSC3kw7AkzLjJ0TtBMclhJeAAm zXj0BD6RpSTa6Pn8JlBDLdCtdExWgdcp+j8e4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=J1eGsGvSupt9keTxPYnbOs6SIupFozzDdx1fHvMuh2A=; b=vJ5ZqbaEimKgbAUs7zVDU2BCsv4mfNewJvxefHCRUg8az5i8BEM+c8/QOBT326FJPM eysvnct3/zPstxn9lFH97jhsXrCBOCF86HpI+67SOqNAv8jyE4OV5NBEGatDj438GWsd aqHSrlOF2nzGc0N6lDdyuutjFFJB05Qv0JcjxemGLcyfg+lLFawrJMoZGkZUsV0cUw4G HQDAABFCrc+qtBdWp8hO0Lu3HNJ3sW1b7r/IZyPIOBP84qNJJJteMSejWrJXACPTZPEN N286kWBja7j9GYd+KBhzCs4aCN/chVRi0PPGKkY37lchAX0Z1vHtWq2e60ry/TDttb4L CteA== X-Gm-Message-State: AOAM530bocbmop7AhQkRVI1EAHG2TTSGzZzqltUMtj5AayiPxYOId+mV 20ixAo+Pk9s1E7zUVSFzzubTSA== X-Google-Smtp-Source: ABdhPJxrsjGQLvgUzp7Jvo+PWFDeMkRjiBjcN4X8rgX4SKtcLuMxjqbB305MCTHD7Lz67nn3YMC7RA== X-Received: by 2002:a05:6512:39d1:: with SMTP id k17mr766967lfu.79.1635150092678; Mon, 25 Oct 2021 01:21:32 -0700 (PDT) Received: from larwa.hq.kempniu.pl ([2001:470:64df:111::e02]) by smtp.gmail.com with ESMTPSA id n13sm239189lfi.127.2021.10.25.01.21.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 25 Oct 2021 01:21:32 -0700 (PDT) From: =?utf-8?b?TWljaGHFgiBLxJlwaWXFhA==?= To: Miquel Raynal , Richard Weinberger , Vignesh Raghavendra Cc: Boris Brezillon , linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org Subject: [PATCH] mtdchar: prevent unbounded allocation in MEMWRITE ioctl Date: Mon, 25 Oct 2021 10:21:04 +0200 Message-Id: <20211025082104.8017-1-kernel@kempniu.pl> X-Mailer: git-send-email 2.33.1 MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211025_012135_685864_57C5F03B X-CRM114-Status: GOOD ( 30.52 ) X-Spam-Score: -0.2 (/) X-Spam-Report: Spam detection software, running on the system "bombadil.infradead.org", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: In the mtdchar_write_ioctl() function, memdup_user() is called with its 'len' parameter set to verbatim values provided by user space via a struct mtd_write_req. Both the 'len' and 'ooblen' fields of [...] Content analysis details: (-0.2 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at https://www.dnswl.org/, no trust [2a00:1450:4864:20:0:0:0:135 listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID_EF Message has a valid DKIM or DK signature from envelope-from domain X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-mtd" Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org In the mtdchar_write_ioctl() function, memdup_user() is called with its 'len' parameter set to verbatim values provided by user space via a struct mtd_write_req. Both the 'len' and 'ooblen' fields of that structure are 64-bit unsigned integers, which means the MEMWRITE ioctl can trigger unbounded kernel memory allocation requests. Fix by iterating over the buffers provided by user space in a loop, processing at most mtd->erasesize bytes in each iteration. Adopt some checks from mtd_check_oob_ops() to retain backward user space compatibility. Suggested-by: Boris Brezillon Signed-off-by: Michał Kępień --- Fixing this problem was suggested last month, during a discussion about a new MEMREAD ioctl. [1] [2] My primary objective was to not break user space, i.e. to not change externally visible behavior compared to the current code. The main problem I faced was that splitting the input data into chunks makes the MEMWRITE ioctl a _wrapper_ for mtd_write_oob() rather than a direct _interface_ to it and yet from the user space perspective it still needs to behave as if nothing changed. Despite my efforts, the patch does _not_ retain absolute backward compatibility and I do not know whether this is acceptable. Specifically, multi-eraseblock-sized writes (requiring multiple loop iterations to be processed) which succeeded with the original code _may_ now return errors and/or write different contents to the device than the original code, depending on the MTD mode of operation requested and on whether the start offset is page-aligned. The documentation for struct mtd_oob_ops warns about even multi-page write requests, but... Example: MTD device parameters: - mtd->writesize = 2048 - mtd->erasesize = 131072 - 64 bytes of raw OOB space per page struct mtd_write_req req = { .start = 2048, .len = 262144, .ooblen = 64, .usr_data = ..., .usr_oob = ..., .mode = MTD_OPS_RAW, }; (This is a 128-page write with OOB data supplied for just one page.) Current mtdchar_write_ioctl() returns 0 for this request and writes 128 pages of data and 1 page's worth of OOB data (64 bytes) to the MTD device. Patched mtdchar_write_ioctl() may return an error because the original request gets split into two chunks (): <131072, 64> <131072, 0> and the second chunk with zero OOB data length may make the MTD driver unhappy in raw mode (resulting in only the first 64 pages of data and 1 page's worth of OOB data getting written to the MTD device). Is an ioctl like this considered a "degenerate" one and therefore acceptable to break or not? At any rate, the revised code feels brittle to me and I would not be particularly surprised if I missed more cases in which it produces different results than the original code. I keep on wondering whether the benefits of this change are worth the extra code complexity, but fortunately it is not my call to make :) Perhaps I am missing something and my proposal can be simplified? Or maybe the way I approached this is completely wrong? Any thoughts on this are welcome. As the outcome of the discussion around this patch will have a significant influence on the shape of the v2 of the MEMREAD ioctl, I decided to submit this one first as a standalone patch. [1] https://lists.infradead.org/pipermail/linux-mtd/2021-September/088485.html [2] https://lists.infradead.org/pipermail/linux-mtd/2021-September/088544.html drivers/mtd/mtdchar.c | 93 ++++++++++++++++++++++++++++++++----------- 1 file changed, 70 insertions(+), 23 deletions(-) diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c index 155e991d9d75..a3afc390e254 100644 --- a/drivers/mtd/mtdchar.c +++ b/drivers/mtd/mtdchar.c @@ -578,9 +578,10 @@ static int mtdchar_write_ioctl(struct mtd_info *mtd, { struct mtd_info *master = mtd_get_master(mtd); struct mtd_write_req req; - struct mtd_oob_ops ops = {}; const void __user *usr_data, *usr_oob; - int ret; + uint8_t *datbuf = NULL, *oobbuf = NULL; + size_t datbuf_len, oobbuf_len; + int ret = 0; if (copy_from_user(&req, argp, sizeof(req))) return -EFAULT; @@ -590,33 +591,79 @@ static int mtdchar_write_ioctl(struct mtd_info *mtd, if (!master->_write_oob) return -EOPNOTSUPP; - ops.mode = req.mode; - ops.len = (size_t)req.len; - ops.ooblen = (size_t)req.ooblen; - ops.ooboffs = 0; - - if (usr_data) { - ops.datbuf = memdup_user(usr_data, ops.len); - if (IS_ERR(ops.datbuf)) - return PTR_ERR(ops.datbuf); - } else { - ops.datbuf = NULL; + + if (!usr_data) + req.len = 0; + + if (!usr_oob) + req.ooblen = 0; + + if (req.start + req.len > mtd->size) + return -EINVAL; + + datbuf_len = min_t(size_t, req.len, mtd->erasesize); + if (datbuf_len > 0) { + datbuf = kmalloc(datbuf_len, GFP_KERNEL); + if (!datbuf) + return -ENOMEM; } - if (usr_oob) { - ops.oobbuf = memdup_user(usr_oob, ops.ooblen); - if (IS_ERR(ops.oobbuf)) { - kfree(ops.datbuf); - return PTR_ERR(ops.oobbuf); + oobbuf_len = min_t(size_t, req.ooblen, mtd->erasesize); + if (oobbuf_len > 0) { + oobbuf = kmalloc(oobbuf_len, GFP_KERNEL); + if (!oobbuf) { + kfree(datbuf); + return -ENOMEM; } - } else { - ops.oobbuf = NULL; } - ret = mtd_write_oob(mtd, (loff_t)req.start, &ops); + while (req.len > 0 || (!usr_data && req.ooblen > 0)) { + struct mtd_oob_ops ops = { + .mode = req.mode, + .len = min_t(size_t, req.len, datbuf_len), + .ooblen = min_t(size_t, req.ooblen, oobbuf_len), + .datbuf = req.len ? datbuf : NULL, + .oobbuf = req.ooblen ? oobbuf : NULL, + }; - kfree(ops.datbuf); - kfree(ops.oobbuf); + /* + * For writes which are not OOB-only, adjust the amount of OOB + * data written according to the number of data pages written. + * This is necessary to prevent OOB data from being skipped + * over in data+OOB writes requiring multiple mtd_write_oob() + * calls to be completed. + */ + if (ops.len > 0 && ops.ooblen > 0) { + u32 oob_per_page = mtd_oobavail(mtd, &ops); + uint32_t pages_to_write = mtd_div_by_ws(ops.len, mtd); + + if (mtd_mod_by_ws(req.start + ops.len, mtd)) + pages_to_write++; + + ops.ooblen = min_t(size_t, ops.ooblen, + pages_to_write * oob_per_page); + } + + if (copy_from_user(datbuf, usr_data, ops.len) || + copy_from_user(oobbuf, usr_oob, ops.ooblen)) { + ret = -EFAULT; + break; + } + + ret = mtd_write_oob(mtd, req.start, &ops); + if (ret) + break; + + req.start += ops.retlen; + req.len -= ops.retlen; + usr_data += ops.retlen; + + req.ooblen -= ops.oobretlen; + usr_oob += ops.oobretlen; + } + + kfree(datbuf); + kfree(oobbuf); return ret; }