[PATCH] mtd-utils: lib: mtd_read: Take the buffer offset into account when reading

[PATCH] mtd-utils: lib: mtd_read: Take the buffer offset into account when reading

[Marcus Prebble]

[-- Attachment #1: Type: text/plain, Size: 246 bytes --]

Hi mtd-list,

Assuming the read() call does not return zero and the result is less
than len, the current implementation will overwrite the data already
read in buf which doesn't seem correct.

Suggested patch attached (git format-patch)

-Marcus

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-mtd_read-Take-the-buffer-offset-into-account-when-re.patch --]
[-- Type: text/x-patch; name="0001-mtd_read-Take-the-buffer-offset-into-account-when-re.patch", Size: 944 bytes --]

From 5dccbbd87604665e896472d52f3351c14c24d2b3 Mon Sep 17 00:00:00 2001
From: Marcus Prebble <prebble@axis.com>
Date: Mon, 5 Oct 2015 17:32:54 +0200
Subject: [PATCH] mtd_read: Take the buffer offset into account when reading

Subsequent calls to read() within the loop will now no longer
overwrite the existing contents of buf.
---
 lib/libmtd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/libmtd.c b/lib/libmtd.c
index 60b4782..bf6d71f 100644
--- a/lib/libmtd.c
+++ b/lib/libmtd.c
@@ -1072,10 +1072,10 @@ int mtd_read(const struct mtd_dev_info *mtd, int fd, int eb, int offs,
 				  mtd->mtd_num, seek);
 
 	while (rd < len) {
-		ret = read(fd, buf, len);
+		ret = read(fd, buf + rd, len - rd);
 		if (ret < 0)
 			return sys_errmsg("cannot read %d bytes from mtd%d (eraseblock %d, offset %d)",
-					  len, mtd->mtd_num, eb, offs);
+					  len - rd, mtd->mtd_num, eb, offs + rd);
 		rd += ret;
 	}
 
-- 
2.1.4

Re: [PATCH] mtd-utils: lib: mtd_read: Take the buffer offset into account when reading

[Brian Norris]

Hi Marcus,

On Tue, Oct 06, 2015 at 02:13:23PM +0200, Marcus Prebble wrote:
> Hi mtd-list,
> 
> Assuming the read() call does not return zero and the result is less
> than len, the current implementation will overwrite the data already
> read in buf which doesn't seem correct.

In addition, this means we might not actually fill up the entire buffer,
since 2 or more short read()'s might get us to exit the loop with a
cumulative value in 'rd', but only a partially-filled buffer. That could
cause a user to try and handle garbage/uninitialized data.

> Suggested patch attached (git format-patch)
> 
> -Marcus

> From 5dccbbd87604665e896472d52f3351c14c24d2b3 Mon Sep 17 00:00:00 2001
> From: Marcus Prebble <prebble@axis.com>
> Date: Mon, 5 Oct 2015 17:32:54 +0200
> Subject: [PATCH] mtd_read: Take the buffer offset into account when reading
> 
> Subsequent calls to read() within the loop will now no longer
> overwrite the existing contents of buf.
> ---
>  lib/libmtd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/libmtd.c b/lib/libmtd.c
> index 60b4782..bf6d71f 100644
> --- a/lib/libmtd.c
> +++ b/lib/libmtd.c
> @@ -1072,10 +1072,10 @@ int mtd_read(const struct mtd_dev_info *mtd, int fd, int eb, int offs,
>  				  mtd->mtd_num, seek);
>  
>  	while (rd < len) {
> -		ret = read(fd, buf, len);
> +		ret = read(fd, buf + rd, len - rd);
>  		if (ret < 0)
>  			return sys_errmsg("cannot read %d bytes from mtd%d (eraseblock %d, offset %d)",
> -					  len, mtd->mtd_num, eb, offs);
> +					  len - rd, mtd->mtd_num, eb, offs + rd);
>  		rd += ret;
>  	}
>  

Patch looks OK. Did you test it? Have you seen MTD drivers that will
return short reads?

Brian

Re: [PATCH] mtd-utils: lib: mtd_read: Take the buffer offset into account when reading

[Marcus Prebble]

Hi Brian,

Thanks for looking at the patch!


On Thu, 2015-11-12 at 11:09 -0800, Brian Norris wrote:
> Hi Marcus,
> 
> On Tue, Oct 06, 2015 at 02:13:23PM +0200, Marcus Prebble wrote:
> > Hi mtd-list,

> ...the current implementation will overwrite the data already
> > read in buf which doesn't seem correct.
> 
> In addition, this means we might not actually fill up the entire buffer,
> since 2 or more short read()'s might get us to exit the loop with a
> cumulative value in 'rd', but only a partially-filled buffer. That could
> cause a user to try and handle garbage/uninitialized data.

Yes, that is definitely possible and not nice for the user.

> Patch looks OK. Did you test it? Have you seen MTD drivers that will
> return short reads?

I only noticed in passing, not because I was hit by the error. I smoke
tested it, but did not hack a driver to return less than len.
If I had to guess, I would say that in nearly all cases the drivers do
not return short reads otherwise this would probably have been picked up
by now. 

> 
> Brian

Re: [PATCH] mtd-utils: lib: mtd_read: Take the buffer offset into account when reading

[Brian Norris]

Hi,

On Fri, Nov 13, 2015 at 11:07:12AM +0100, Marcus Prebble wrote:
> Thanks for looking at the patch!

Thanks for the patch!

> On Thu, 2015-11-12 at 11:09 -0800, Brian Norris wrote:
> > Patch looks OK. Did you test it? Have you seen MTD drivers that will
> > return short reads?
> 
> I only noticed in passing, not because I was hit by the error. I smoke
> tested it, but did not hack a driver to return less than len.
> If I had to guess, I would say that in nearly all cases the drivers do
> not return short reads otherwise this would probably have been picked up
> by now. 

OK, that's fine. It's good to know when things are likely to cause real
problems vs. when things need fixed just for best practice.

BTW, I noticed there isn't a 'Signed-off-by' tag in the patch. It's
mostly a formality, but we really shouldn't be taking patches without
it. Can you paste one in reply to your patch, and I'll C&P it?

Regards,
Brian

Re: [PATCH] mtd-utils: lib: mtd_read: Take the buffer offset into account when reading

[Marcus Prebble]

Hi again and sorry for the delay in replying!


On Fri, 2015-11-13 at 11:17 -0800, Brian Norris wrote:

> BTW, I noticed there isn't a 'Signed-off-by' tag in the patch. 
>  Can you paste one in reply to your patch, and I'll C&P it?
> 

Signed-off-by: Marcus Prebble <marcus.prebble@axis.com>


Kind regards,
-Marcus


> Regards,
> Brian

Re: [PATCH] mtd-utils: lib: mtd_read: Take the buffer offset into account when reading

[Brian Norris]

On Tue, Nov 17, 2015 at 09:43:40AM +0100, Marcus Prebble wrote:
> On Fri, 2015-11-13 at 11:17 -0800, Brian Norris wrote:
> 
> > BTW, I noticed there isn't a 'Signed-off-by' tag in the patch. 
> >  Can you paste one in reply to your patch, and I'll C&P it?
> > 
> 
> Signed-off-by: Marcus Prebble <marcus.prebble@axis.com>

Pushed to mtd-utils.git. Thanks!