linux-kernel - Re: [PATCH 1/3] MTD: m25p80: fix write return value.

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 for Android: free password hash cracker in your pocket

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

Date:	Wed, 20 May 2015 16:45:25 -0700
From:	Brian Norris <computersforpeace@...il.com>
To:	Michal Suchanek <hramrach@...il.com>
Cc:	David Woodhouse <dwmw2@...radead.org>, zajec5@...il.com,
	Marek Vasut <marex@...x.de>,
	Alison Chaiken <alison_chaiken@...tor.com>,
	Ben Hutchings <ben@...adent.org.uk>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	"Bean Huo (beanhuo)" <beanhuo@...ron.com>,
	"grmoore@...era.com" <grmoore@...era.com>,
	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] MTD: m25p80: fix write return value.

On Thu, Apr 30, 2015 at 03:33:47PM +0200, Michal Suchanek wrote:
> The 'retlen' points to a variable representing the number of data bytes
> written/read (see include/linux/mtd/mtd.h) by the current invocation of
> the function. This variable must be set, not incremented.
> 
> v2: clearer commit message
> 
> Signed-off-by: Michal Suchanek <hramrach@...il.com>
> Acked-by: Marek Vasut <marex@...x.de>
> ---
>  drivers/mtd/devices/m25p80.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7c8b169..0b2bc21 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>  
>  	spi_sync(spi, &m);
>  
> -	*retlen += m.actual_length - cmd_sz;
> +	*retlen = m.actual_length - cmd_sz;

This is very wrong. It gets a little better once you add your next
patches (which are also not good) since those patches reinterpret the
usage of retlen, but this one definitely does *not* stand a lone.

I'll admit the API is a little odd here, but the callers of this
function (see spi_nor_write()) actually depend on calling this multiple
times, with the value incrementing each time so we get a summary total.
You're now clobbering this value.

I'm willing to accept patches to improve this API, if you think that
would help. Or to add comments that document this.

>  }
>  
>  static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

Also, I'm a little confused because you sent two different patch series
very close to each other, and this patch is in both of them. Please
don't do that. Either send a single patch series that contains all
patches (and is versions v2, v3, etc., as you revise the whole thing) or
send completely independent patch series. Don't include the same patch
in different series.

Anyway, please consider my comments, and when you have something better,
please resend everything. I'm not going to take either series as-is.

Thanks,
Brian
--
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/