linux-kernel - Re: [PATCH 1/1] mtd: nand: add check for out of page read

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] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 14 Dec 2010 17:12:37 +0200
From:	Artem Bityutskiy <dedekind1@...il.com>
To:	Jason Liu <r64343@...escale.com>
Cc:	David.Woodhouse@...el.com, linux-mtd@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] mtd: nand: add check for out of page read

On Fri, 2010-11-19 at 16:40 +0800, Jason Liu wrote:
> When run mtd_oobtest case, there will be one error for step(4),
> which turned out it need add one check for out of page read in
> nand_do_read_oob just like mtd_do_write_oob did it already.
> This commit also fix one typo error for comments in mtd_do_write_oob
> 
> Signed-off-by: Jason Liu <r64343@...escale.com>
> ---
>  drivers/mtd/nand/nand_base.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 1f75a1b..75d199e 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1782,6 +1782,13 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
>  	else
>  		len = mtd->oobsize;
>  
> +	/* Do not allow read past end of page */
> +	if ((ops->ooboffs + readlen) > len) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to read "
> +				"past end of page\n", __func__);
> +		return -EINVAL;
> +	}

As you reported to me in a private e-mail (although I prefer to always
have a public ML in CC when dealing with open source stuff) this patch
is wrong. Indeed, it limits the maximum amount of bytes which can be
read at one go to the OOB size, which is incorrect, because
mtd->read_oob() allows reading multiple pages at a time, see comment
near "struct mtd_oob_ops" at include/linux/mtd/mtd.h. So this patch
breaks ABI and hence, has to be reverted.

>  	if (unlikely(ops->ooboffs >= len)) {
>  		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to start read "
>  					"outside oob\n", __func__);

Side note: nand_base.c has a bunch of senseless unlikely() hints, would
be nice to clean that up some day.

> -	/* Do not allow reads past end of device */
> +	/* Do not allow write past end of device */

Care to make this a separate clean-up patch meanwhile?

Thank!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/