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

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@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;

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

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@denx.de

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

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@denx.de
participants (3)
-
Ben Gardiner
-
Stefan Roese
-
Wolfgang Denk