[U-Boot] [PATCH] UBIFS: Change ubifsload to not read beyond the requested size

[U-Boot] [PATCH] UBIFS: Change ubifsload to not read beyond the requested size

[Stefan Roese]

Until now ubifsload pads the destination with 0 up to a multiple of
UBIFS_BLOCK_SIZE (4KiB) while reading a file to memory. This patch
changes this behaviour to only read to the requested length. This
is either the file length or the length/size provided as parameter
to the ubifsload command.

Signed-off-by: Stefan Roese <sr@denx.de>
---
This patch makes the patch [UBIFS: Add padded size to ubifsload output]
posted yesterday obsolete since we're not padding any more now.

Stefan

 fs/ubifs/ubifs.c |   58 ++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index 8215443..e49ae6f 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -3,7 +3,7 @@
  *
  * Copyright (C) 2006-2008 Nokia Corporation.
  *
- * (C) Copyright 2008-2009
+ * (C) Copyright 2008-2010
  * Stefan Roese, DENX Software Engineering, sr at denx.de.
  *
  * This program is free software; you can redistribute it and/or modify it
@@ -567,7 +567,8 @@ dump:
 	return -EINVAL;
 }
 
-static int do_readpage(struct ubifs_info *c, struct inode *inode, struct page *page)
+static int do_readpage(struct ubifs_info *c, struct inode *inode,
+		       struct page *page, int last_block_size)
 {
 	void *addr;
 	int err = 0, i;
@@ -601,17 +602,39 @@ static int do_readpage(struct ubifs_info *c, struct inode *inode, struct page *p
 			err = -ENOENT;
 			memset(addr, 0, UBIFS_BLOCK_SIZE);
 		} else {
-			ret = read_block(inode, addr, block, dn);
-			if (ret) {
-				err = ret;
-				if (err != -ENOENT)
-					break;
-			} else if (block + 1 == beyond) {
-				int dlen = le32_to_cpu(dn->size);
-				int ilen = i_size & (UBIFS_BLOCK_SIZE - 1);
-
-				if (ilen && ilen < dlen)
-					memset(addr + ilen, 0, dlen - ilen);
+			/*
+			 * Reading last block? Make sure to not write beyond
+			 * the requested size.
+			 */
+			if (((block + 1) == beyond) || last_block_size) {
+				void *buff;
+				int dlen;
+
+				/*
+				 * We need to buffer the data locally for the
+				 * last block. This is to not pad the destination
+				 * area to a multiple of UBIFS_BLOCK_SIZE.
+				 */
+				buff = malloc(UBIFS_BLOCK_SIZE);
+
+				/* Read block-siez into temp buffer */
+				ret = read_block(inode, buff, block, dn);
+				if (last_block_size)
+					dlen = last_block_size;
+				else
+					dlen = le32_to_cpu(dn->size);
+
+				/* Now copy required size back to dest */
+				memcpy(addr, buff, dlen);
+
+				free(buff);
+			} else {
+				ret = read_block(inode, addr, block, dn);
+				if (ret) {
+					err = ret;
+					if (err != -ENOENT)
+						break;
+				}
 			}
 		}
 		if (++i >= UBIFS_BLOCKS_PER_PAGE)
@@ -649,6 +672,7 @@ int ubifs_load(char *filename, u32 addr, u32 size)
 	int err = 0;
 	int i;
 	int count;
+	int last_block_size = 0;
 
 	c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY);
 	/* ubifs_findfile will resolve symlinks, so we know that we get
@@ -684,7 +708,13 @@ int ubifs_load(char *filename, u32 addr, u32 size)
 	page.index = 0;
 	page.inode = inode;
 	for (i = 0; i < count; i++) {
-		err = do_readpage(c, inode, &page);
+		/*
+		 * Make sure to not read beyond the requested size
+		 */
+		if (((i + 1) == count) && (size < inode->i_size))
+			last_block_size = size - (i * PAGE_SIZE);
+
+		err = do_readpage(c, inode, &page, last_block_size);
 		if (err)
 			break;
 
-- 
1.7.3.2

[U-Boot] [PATCH] UBIFS: Change ubifsload to not read beyond the requested size

[Ben Gardiner]

On Fri, Oct 29, 2010 at 5:04 AM, Stefan Roese <sr@denx.de> wrote:
> Until now ubifsload pads the destination with 0 up to a multiple of
> UBIFS_BLOCK_SIZE (4KiB) while reading a file to memory. This patch
> changes this behaviour to only read to the requested length. This
> is either the file length or the length/size provided as parameter
> to the ubifsload command.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> ---
> [...]
> + ? ? ? ? ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ? ? ? ? ?* Reading last block? Make sure to not write beyond
> + ? ? ? ? ? ? ? ? ? ? ? ?* the requested size.
> + ? ? ? ? ? ? ? ? ? ? ? ?*/
s/write/read/

> [...]
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /* Read block-siez into temp buffer */
s/siez/size/

Applies to 908614f20f7f0f5df736eed21b88e81ebbf14e86 of
git://git.denx.de/u-boot.git.

One checkpatch warning:

WARNING: line over 80 characters
#134: FILE: fs/ubifs/ubifs.c:615:
+                                * last block. This is to not pad the
destination

total: 0 errors, 1 warnings, 88 lines checked

Tested on da850evm with NAND, env.oob enabled. Loaded a uImage from
ubifs and booted it.

Tested-by: Ben Gardiner <bengardiner@nanometrics.ca>

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

[U-Boot] [PATCH] UBIFS: Change ubifsload to not read beyond the requested size

[Wolfgang Denk]

Dear Stefan Roese,

In message <1288343093-31276-1-git-send-email-sr@denx.de> you wrote:
> Until now ubifsload pads the destination with 0 up to a multiple of
> UBIFS_BLOCK_SIZE (4KiB) while reading a file to memory. This patch
> changes this behaviour to only read to the requested length. This
> is either the file length or the length/size provided as parameter
> to the ubifsload command.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> ---
> This patch makes the patch [UBIFS: Add padded size to ubifsload output]
> posted yesterday obsolete since we're not padding any more now.
...
> +				/*
> +				 * We need to buffer the data locally for the
> +				 * last block. This is to not pad the destination
> +				 * area to a multiple of UBIFS_BLOCK_SIZE.
> +				 */
> +				buff = malloc(UBIFS_BLOCK_SIZE);
> +
> +				/* Read block-siez into temp buffer */
> +				ret = read_block(inode, buff, block, dn);
> +				if (last_block_size)
> +					dlen = last_block_size;
> +				else
> +					dlen = le32_to_cpu(dn->size);

How about some error handling?

malloc() can fail, and read_block() can fail as well.

> +				/* Now copy required size back to dest */
> +				memcpy(addr, buff, dlen);
> +
> +				free(buff);
> +			} else {
> +				ret = read_block(inode, addr, block, dn);
> +				if (ret) {
> +					err = ret;
> +					if (err != -ENOENT)
> +						break;

or simpler:

				ret = read_block(inode, addr, block, dn);
				if (ret != -ENOENT) {
					err = ret; 
					break;
				}


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I used to think that the brain was the most wonderful  organ  in  my
body. Then I realized who was telling me this."        - Emo Phillips

[U-Boot] [PATCH] UBIFS: Change ubifsload to not read beyond the requested size

[Stefan Roese]

Hi Wolfgang,

On Friday 29 October 2010 22:54:04 Wolfgang Denk wrote:
> > +				buff = malloc(UBIFS_BLOCK_SIZE);
> > +
> > +				/* Read block-siez into temp buffer */
> > +				ret = read_block(inode, buff, block, dn);
> > +				if (last_block_size)
> > +					dlen = last_block_size;
> > +				else
> > +					dlen = le32_to_cpu(dn->size);
> 
> How about some error handling?
> 
> malloc() can fail, and read_block() can fail as well.

Right. I'll change this and send an updated version.
 
Thanks for the review.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

[U-Boot] [PATCH] UBIFS: Change ubifsload to not read beyond the requested size

[Stefan Roese]

Hi Ben,

On Friday 29 October 2010 16:01:18 Ben Gardiner wrote:
> On Fri, Oct 29, 2010 at 5:04 AM, Stefan Roese <sr@denx.de> wrote:
> > Until now ubifsload pads the destination with 0 up to a multiple of
> > UBIFS_BLOCK_SIZE (4KiB) while reading a file to memory. This patch
> > changes this behaviour to only read to the requested length. This
> > is either the file length or the length/size provided as parameter
> > to the ubifsload command.
> > 
> > Signed-off-by: Stefan Roese <sr@denx.de>
> > ---
> > [...]
> > +                       /*
> > +                        * Reading last block? Make sure to not write
> > beyond +                        * the requested size.
> > +                        */
> 
> s/write/read/

No. This refers to not writing beyond the requested size in the destination 
buffer. So that nothing will be overwritten by this 4KiB padding. I'll change 
this comment a bit to make this clearer in the next patch version.
 
> > [...]
> > +                               /* Read block-siez into temp buffer */
> 
> s/siez/size/

Thanks for catching. Will change.
 
Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de