[U-Boot] [PATCH v2] cmd: usb_mass_storage: add protection for block_dev

[Patrick DELAUNAY]

Hi,

> From: Marek Vasut <marex at denx.de>
> 
> On 09/26/2018 01:04 PM, Patrick Delaunay wrote:
> > solve data abort for the command "ums 0 ubi 0"
> > because result of case blk_get_device_part_str() result is OK but with
> > block_dev = 0 when CONFIG_CMD_UBIFS is activate and ubi volume is
> > mounted
> 
> I don't quite understand the commit message, can you reword it?

Ok, I prepare a V3 
 
> Also, when is this ever called with block_dev == NULL ? What's the condition to
> trigger this and why ?

To reproduce the issue you need to have a ubifs  already mounted,
for example with the command:
	ubi part UBI
 	ubifsmount ubi0:boot

I investigate the point, the call stack before the crash is  caused by the ubi support in 
./disk/part.c:425 =  blk_get_device_part_str()

Some part of code here are under CONFIG_CMD_UBIFS with the comment :
	"ubi goes through a mtd, rathen then through a regular block device"

So the the function return a pointer to disk_partition_t :
		info->type = BOOT_PART_TYPE
		info->name =  "UBI"
 but without block device information !
		*dev_desc = NULL

So the issue occurs because, when the ubifs volume is mounted,
The code in  cmd/usb_mass_storage.c

static int ums_init(const char *devtype, const char *devnums_part_str)
{
...
	for (;;) {
...
		partnum = blk_get_device_part_str(devtype, devnum_part_str,
					&block_dev, &info, 1);
....
// With devnum_part_str  = "ubi 0"
// This function don't return a error and return a  valid partnum
// So the function continue until block_dev = NULL is used
.....
		if (partnum < 0)
			goto cleanup;

		/* Check if the argument is in legacy format. If yes,
		 * expose all partitions by setting the partnum = 0
		 * e.g. ums 0 mmc 0
		 */
		if (!strchr(devnum_part_str, ':'))
			partnum = 0;

		/* f_mass_storage.c assumes SECTOR_SIZE sectors */
		if (block_dev->blksz != SECTOR_SIZE)
			goto cleanup;

=> crash occur here (on my board) because block_dev = NULL

> > Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> > ---
> > I check and the issue is still present on U-Boot v2018.09, on my board
> > stm32mp1 (with NAND device and UBI volume):
> >
> > STM32MP> ubi part UBI
> > STM32MP> ubifsmount ubi0:boot
> > STM32MP> ums 0 ubi 0
> > data abort
> > pc : [<ffc60abc>]	   lr : [<ffc60ab3>]
> > reloc pc : [<c0110abc>]	   lr : [<c0110ab3>]
> > sp : fdc3e460  ip : fdc3e518	 fp : fdcf06a8
> > r10: fdcf06b8  r9 : fdc4eed8	 r8 : 00000000
> > r7 : ffce3d84  r6 : fdcf0740	 r5 : fdcf0740  r4 : ffce3d88
> > r3 : 00000000  r2 : 00000000	 r1 : 0000003a  r0 : 00000000
> > Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
> > Code: f04f4628 9b06fd2a bf082800 0800f04f (f5b3695b) Resetting CPU ...
> >
> > Even If the command is invalid (ums not allowed on ubi device), the
> > data abort can be avoid by this patch.
> >
> >
> > Changes in v2:
> >     - Rebase on master branch
> >
> >  cmd/usb_mass_storage.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/cmd/usb_mass_storage.c b/cmd/usb_mass_storage.c index
> > 26b41b4c4..a3a338c 100644
> > --- a/cmd/usb_mass_storage.c
> > +++ b/cmd/usb_mass_storage.c
> > @@ -85,7 +85,7 @@ static int ums_init(const char *devtype, const char
> *devnums_part_str)
> >  			partnum = 0;
> >
> >  		/* f_mass_storage.c assumes SECTOR_SIZE sectors */
> > -		if (block_dev->blksz != SECTOR_SIZE)
> > +		if (!block_dev || block_dev->blksz != SECTOR_SIZE)
> >  			goto cleanup;
> >
> >  		ums_new = realloc(ums, (ums_count + 1) * sizeof(*ums));
> >
> 
> 
> --
> Best regards,
> Marek Vasut

Regards
Patrick