From: "Michał Kępień" <kernel@kempniu.pl> To: Miquel Raynal <miquel.raynal@bootlin.com> Cc: Richard Weinberger <richard@nod.at>, Vignesh Raghavendra <vigneshr@ti.com>, Boris Brezillon <boris.brezillon@collabora.com>, linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mtdchar: prevent unbounded allocation in MEMWRITE ioctl Date: Fri, 26 Nov 2021 14:02:33 +0100 [thread overview] Message-ID: <YaDa6Znbms3tVkr/@larwa.hq.kempniu.pl> (raw) In-Reply-To: <20211126103116.5bef6bc0@xps13> Hi Miquèl, > > 5. The above causes the driver to receive 2048 bytes of data for a page > > write in raw mode, which results in an error that propagates all the > > way up to mtdchar_write_ioctl(). > > This is definitely far from an expected behavior. Writing a page > without OOB is completely fine. Could it be a nandsim quirk? Sorry, I do not feel qualified enough to comment on this. I prepared a code flow analysis below, though. > > The nandsim driver returns the same error if you pass the following > > request to the MEMWRITE ioctl: > > > > struct mtd_write_req req = { > > .start = 2048, > > .len = 2048, > > .ooblen = 0, > > .usr_data = 0x10000000, > > .usr_oob = NULL, > > .mode = MTD_OPS_RAW, > > }; > > > > so it is not the driver that is broken or insane, it is the splitting > > process that may cause the MEMWRITE ioctl to return different error > > codes than before. > > > > I played with the code a bit more and I found a fix which addresses this > > issue without breaking other scenarios: setting oobbuf to the same > > pointer for every loop iteration (if ooblen is 0, no OOB data will be > > written anyway). > > You mean that > { .user_oob = NULL, .ooblen = 0 } > fails, while > { .user_oob = random, .ooblen = 0 } > works? This seems a little bit fragile. That is indeed the behavior I am observing with nandsim, even on a kernel which does not include my patch. > Could you tell us the origin of the error? Because in > nand_do_write_ops() if ops->oobbuf is populated then oob_required is > set to true no matter the value set in ooblen. Correct - and that is what causes the behavior described above (and why the tweak I came up with works around the problem). nand_do_write_ops() calls nand_write_page() with 'oob_required' passed as the fifth parameter. In raw mode, nand_write_page() calls nand_write_page_raw(). Here is what happens there: 1. nand_prog_page_begin_op() sets up a page programming operation by sending a few commands to the chip. See nand_exec_prog_page_op() for details. Note that since the 'prog' parameter is set to false, the last two instructions from the 'instrs' array are not run yet. 2. 'oob_required' is checked. If it is set to 1, OOB data is sent to the chip; otherwise, it is not sent. 3. nand_prog_page_end_op() is called to finish the programming operation. At that point, the ACTION_PRGPAGE switch case in ns_do_state_action() (in drivers/mtd/nand/raw/nandsim.c) checks whether the number of bytes it received so far for this operation (ns->regs.count, updated by ns_nand_write_buf() as data is pushed to the chip) equals the number of bytes in a full page with OOB data (num). If not, an error is returned, which results in the NAND_STATUS_FAIL flag being set in the status byte, triggering an -EIO. This does not happen for any other MTD operation mode because the chip callbacks that nand_write_page() invokes in those other modes cause OOB data to be sent to the chip. > Plus, the code in mtdchar is clear: .oobbuf is set to NULL if there are > no OOBs provided by the user so I believe this is a situation that > should already work. Correct, though current mtdchar_write_ioctl() code only looks at the value of the 'usr_oob' field in the struct mtd_write_req passed to it, so even if you pass { .usr_oob = <something non-NULL>, .ooblen = 0 }, it will still set ops.oobbuf to the pointer returned by memdup_user(). -- Best regards, Michał Kępień ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
WARNING: multiple messages have this Message-ID (diff)
From: "Michał Kępień" <kernel@kempniu.pl> To: Miquel Raynal <miquel.raynal@bootlin.com> Cc: Richard Weinberger <richard@nod.at>, Vignesh Raghavendra <vigneshr@ti.com>, Boris Brezillon <boris.brezillon@collabora.com>, linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mtdchar: prevent unbounded allocation in MEMWRITE ioctl Date: Fri, 26 Nov 2021 14:02:33 +0100 [thread overview] Message-ID: <YaDa6Znbms3tVkr/@larwa.hq.kempniu.pl> (raw) In-Reply-To: <20211126103116.5bef6bc0@xps13> Hi Miquèl, > > 5. The above causes the driver to receive 2048 bytes of data for a page > > write in raw mode, which results in an error that propagates all the > > way up to mtdchar_write_ioctl(). > > This is definitely far from an expected behavior. Writing a page > without OOB is completely fine. Could it be a nandsim quirk? Sorry, I do not feel qualified enough to comment on this. I prepared a code flow analysis below, though. > > The nandsim driver returns the same error if you pass the following > > request to the MEMWRITE ioctl: > > > > struct mtd_write_req req = { > > .start = 2048, > > .len = 2048, > > .ooblen = 0, > > .usr_data = 0x10000000, > > .usr_oob = NULL, > > .mode = MTD_OPS_RAW, > > }; > > > > so it is not the driver that is broken or insane, it is the splitting > > process that may cause the MEMWRITE ioctl to return different error > > codes than before. > > > > I played with the code a bit more and I found a fix which addresses this > > issue without breaking other scenarios: setting oobbuf to the same > > pointer for every loop iteration (if ooblen is 0, no OOB data will be > > written anyway). > > You mean that > { .user_oob = NULL, .ooblen = 0 } > fails, while > { .user_oob = random, .ooblen = 0 } > works? This seems a little bit fragile. That is indeed the behavior I am observing with nandsim, even on a kernel which does not include my patch. > Could you tell us the origin of the error? Because in > nand_do_write_ops() if ops->oobbuf is populated then oob_required is > set to true no matter the value set in ooblen. Correct - and that is what causes the behavior described above (and why the tweak I came up with works around the problem). nand_do_write_ops() calls nand_write_page() with 'oob_required' passed as the fifth parameter. In raw mode, nand_write_page() calls nand_write_page_raw(). Here is what happens there: 1. nand_prog_page_begin_op() sets up a page programming operation by sending a few commands to the chip. See nand_exec_prog_page_op() for details. Note that since the 'prog' parameter is set to false, the last two instructions from the 'instrs' array are not run yet. 2. 'oob_required' is checked. If it is set to 1, OOB data is sent to the chip; otherwise, it is not sent. 3. nand_prog_page_end_op() is called to finish the programming operation. At that point, the ACTION_PRGPAGE switch case in ns_do_state_action() (in drivers/mtd/nand/raw/nandsim.c) checks whether the number of bytes it received so far for this operation (ns->regs.count, updated by ns_nand_write_buf() as data is pushed to the chip) equals the number of bytes in a full page with OOB data (num). If not, an error is returned, which results in the NAND_STATUS_FAIL flag being set in the status byte, triggering an -EIO. This does not happen for any other MTD operation mode because the chip callbacks that nand_write_page() invokes in those other modes cause OOB data to be sent to the chip. > Plus, the code in mtdchar is clear: .oobbuf is set to NULL if there are > no OOBs provided by the user so I believe this is a situation that > should already work. Correct, though current mtdchar_write_ioctl() code only looks at the value of the 'usr_oob' field in the struct mtd_write_req passed to it, so even if you pass { .usr_oob = <something non-NULL>, .ooblen = 0 }, it will still set ops.oobbuf to the pointer returned by memdup_user(). -- Best regards, Michał Kępień
next prev parent reply other threads:[~2021-11-26 13:03 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-25 8:21 [PATCH] mtdchar: prevent unbounded allocation in MEMWRITE ioctl Michał Kępień 2021-10-25 8:21 ` Michał Kępień 2021-11-22 9:31 ` Miquel Raynal 2021-11-22 9:31 ` Miquel Raynal 2021-11-25 12:08 ` Michał Kępień 2021-11-25 12:08 ` Michał Kępień 2021-11-26 9:31 ` Miquel Raynal 2021-11-26 9:31 ` Miquel Raynal 2021-11-26 13:02 ` Michał Kępień [this message] 2021-11-26 13:02 ` Michał Kępień
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YaDa6Znbms3tVkr/@larwa.hq.kempniu.pl \ --to=kernel@kempniu.pl \ --cc=boris.brezillon@collabora.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mtd@lists.infradead.org \ --cc=miquel.raynal@bootlin.com \ --cc=richard@nod.at \ --cc=vigneshr@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.