[U-Boot] [PATCH] part_dos: allocate sector buffer dynamically

Apple iPod nanos have sector sizes of 2 or 4 KiB, which crashes U-Boot when it tries to read the MBR into 512-byte buffer situated on stack. Instead allocate this buffer dynamically to be safe with any large sector size.
Signed-off-by: Sergei Shtylyov sshtylyov@ru.mvista.com
--- The same change is probably needed for disk/part_amiga.c but I'm not really sure if Amiga supports USB... :-)
disk/part_dos.c | 59 +++++++++++++++++++++++++++++++++++++++----------------- disk/part_dos.h | 7 ------ 2 files changed, 42 insertions(+), 24 deletions(-)
Index: u-boot/disk/part_dos.c =================================================================== --- u-boot.orig/disk/part_dos.c +++ u-boot/disk/part_dos.c @@ -32,6 +32,7 @@
#include <common.h> #include <command.h> +#include <exports.h> #include <ide.h> #include "part_dos.h"
@@ -87,14 +88,20 @@ static int test_block_type(unsigned char
int test_part_dos (block_dev_desc_t *dev_desc) { - unsigned char buffer[DEFAULT_SECTOR_SIZE]; + unsigned char *buffer; + + buffer = malloc(dev_desc->blksz); + if (!buffer) + return -1;
if ((dev_desc->block_read(dev_desc->dev, 0, 1, (ulong *) buffer) != 1) || (buffer[DOS_PART_MAGIC_OFFSET + 0] != 0x55) || (buffer[DOS_PART_MAGIC_OFFSET + 1] != 0xaa) ) { - return (-1); + free(buffer); + return -1; } - return (0); + free(buffer); + return 0; }
/* Print a partition that is relative to its Extended partition table @@ -102,26 +109,32 @@ int test_part_dos (block_dev_desc_t *dev static void print_partition_extended (block_dev_desc_t *dev_desc, int ext_part_sector, int relative, int part_num) { - unsigned char buffer[DEFAULT_SECTOR_SIZE]; + unsigned char *buffer; dos_partition_t *pt; int i;
+ buffer = malloc(dev_desc->blksz); + if (!buffer) { + printf ("** Can't allocate buffer **\n"); + return; + } + if (dev_desc->block_read(dev_desc->dev, ext_part_sector, 1, (ulong *) buffer) != 1) { printf ("** Can't read partition table on %d:%d **\n", dev_desc->dev, ext_part_sector); - return; + goto exit; } i=test_block_type(buffer); if(i==-1) { printf ("bad MBR sector signature 0x%02x%02x\n", buffer[DOS_PART_MAGIC_OFFSET], buffer[DOS_PART_MAGIC_OFFSET + 1]); - return; + goto exit; } if(i==DOS_PBR) { printf (" 1\t\t 0\t%10ld\t%2x\n", dev_desc->lba, buffer[DOS_PBR_MEDIA_TYPE_OFFSET]); - return; + goto exit; } /* Print all primary/logical partitions */ pt = (dos_partition_t *) (buffer + DOS_PART_TBL_OFFSET); @@ -156,7 +169,8 @@ static void print_partition_extended (bl } }
- return; +exit: + free(buffer); }
@@ -166,21 +180,27 @@ static int get_partition_info_extended ( int relative, int part_num, int which_part, disk_partition_t *info) { - unsigned char buffer[DEFAULT_SECTOR_SIZE]; + unsigned char *buffer; dos_partition_t *pt; - int i; + int i, result = -1; + + buffer = malloc(dev_desc->blksz); + if (!buffer) { + printf ("** Can't allocate buffer **\n"); + return -1; + }
if (dev_desc->block_read (dev_desc->dev, ext_part_sector, 1, (ulong *) buffer) != 1) { printf ("** Can't read partition table on %d:%d **\n", dev_desc->dev, ext_part_sector); - return -1; + goto exit; } if (buffer[DOS_PART_MAGIC_OFFSET] != 0x55 || buffer[DOS_PART_MAGIC_OFFSET + 1] != 0xaa) { printf ("bad MBR sector signature 0x%02x%02x\n", buffer[DOS_PART_MAGIC_OFFSET], buffer[DOS_PART_MAGIC_OFFSET + 1]); - return -1; + goto exit; }
/* Print all primary/logical partitions */ @@ -223,7 +243,8 @@ static int get_partition_info_extended ( } /* sprintf(info->type, "%d, pt->sys_ind); */ sprintf ((char *)info->type, "U-Boot"); - return 0; + result = 0; + goto exit; }
/* Reverse engr the fdisk part# assignment rule! */ @@ -239,12 +260,16 @@ static int get_partition_info_extended ( if (is_extended (pt->sys_ind)) { int lba_start = le32_to_int (pt->start4) + relative;
- return get_partition_info_extended (dev_desc, lba_start, - ext_part_sector == 0 ? lba_start : relative, - part_num, which_part, info); + result = get_partition_info_extended(dev_desc, + lba_start, ext_part_sector == 0 ? + lba_start : relative, + part_num, which_part, info); + goto exit; } } - return -1; +exit: + free(buffer); + return result; }
void print_part_dos (block_dev_desc_t *dev_desc) Index: u-boot/disk/part_dos.h =================================================================== --- u-boot.orig/disk/part_dos.h +++ u-boot/disk/part_dos.h @@ -25,13 +25,6 @@ #define _DISK_PART_DOS_H
-#ifdef CONFIG_ISO_PARTITION -/* Make the buffers bigger if ISO partition support is enabled -- CD-ROMS - have 2048 byte blocks */ -#define DEFAULT_SECTOR_SIZE 2048 -#else -#define DEFAULT_SECTOR_SIZE 512 -#endif #define DOS_PART_TBL_OFFSET 0x1be #define DOS_PART_MAGIC_OFFSET 0x1fe #define DOS_PBR_FSTYPE_OFFSET 0x36

Dear Sergei Shtylyov,
In message 201104122323.59105.sshtylyov@ru.mvista.com you wrote:
Apple iPod nanos have sector sizes of 2 or 4 KiB, which crashes U-Boot when it tries to read the MBR into 512-byte buffer situated on stack. Instead allocate this buffer dynamically to be safe with any large sector size.
Signed-off-by: Sergei Shtylyov sshtylyov@ru.mvista.com
Can we please keep the buffer on the stack as before? There is no need to use malloc() here. It just makes the code slower and more complicated and error prone without neeed.
Best regards,
Wolfgang Denk

Hello.
On 30-04-2011 23:14, Wolfgang Denk wrote:
Apple iPod nanos have sector sizes of 2 or 4 KiB, which crashes U-Boot when it tries to read the MBR into 512-byte buffer situated on stack. Instead allocate this buffer dynamically to be safe with any large sector size.
Signed-off-by: Sergei Shtylyovsshtylyov@ru.mvista.com
Can we please keep the buffer on the stack as before?
It will be unsafe. We can't really predict the size of the buffer (unless we postulate that the buffer size won't ever exceed e.g. 4K).
There is no need to use malloc() here.
No, there *is* a need I think.
It just makes the code slower and more complicated and error prone without neeed.
I think using stack variables makes the code much more error prone, to the point that U-Boot just crashes when the sector size happens to exceed our buffer size.
Best regards,
Wolfgang Denk
WBR, Sergei

Dear Sergei Shtylyov,
In message 4DBFF300.9010906@mvista.com you wrote:
Can we please keep the buffer on the stack as before?
It will be unsafe. We can't really predict the size of the buffer (unless
we postulate that the buffer size won't ever exceed e.g. 4K).
In which way will a buffer allocated on the stack be less safe than one allocated using malloc()? Changes to do things wrong (like forgetting to free the array on return or freeing a bad pointer) are much higher with malloc(), it seems.
I think using stack variables makes the code much more error prone, to the
point that U-Boot just crashes when the sector size happens to exceed our buffer size.
This statement makes no sense to me. Wether the sector size exceeds the buffer size or not is in no way dependent on where or how you allocate the buffer - be it on the stack or using malloc().
Umm... you _are_ aware that you can put dynamically sized arrays on the stack, aren't you?
Best regards,
Wolfgang Denk

Dear Sergei Shtylyov,
In message 4DBFF9FD.1070907@mvista.com you wrote:
Umm... you _are_ aware that you can put dynamically sized arrays on the stack, aren't you?
No, it seems I'm not. Is it a standard C now?
It's been a GCC extension forever (well, I have to admit that I don't remember before GCC v1.39 or so).
Best regards,
Wolfgang Denk
participants (3)
-
Sergei Shtylyov
-
Sergei Shtylyov
-
Wolfgang Denk