[U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation

[Lukasz Majewski <l.majewski@samsung.com>]

Hi Tom,

> On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote:
> > Support for MMC storage devices to work with DFU framework.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > Cc: Marek Vasut <marex@denx.de>
> [snip]
> > +	case RAW_ADDR:
> > +		sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned
> > int) buf,
> > +			dfu->data.mmc.lba_start,
> > dfu->data.mmc.lba_size);
> > +		break;
> > +	case FAT:
> > +		sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx",
> > +			dfu->data.mmc.dev, dfu->data.mmc.part,
> > +			(unsigned int) buf, dfu->name, *len);
> > +		break;
> > +	default:
> > +		printf("%s: Wrong layout!\n", __func__);
> > +	}
> > +
> > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > +	run_command(cmd_buf, 0);
> 
> If we try and take the long-view here, that fatwrite/mmc write don't
> perform a lot of sanity checking on input isn't good.  Lots of
> commands I believe don't, but we can start somewhere. 
Yes, indeed they don't. But I think, that it is a deeper problem.

When one looks into the cmd_mmc.c, the code is not checking the
correctness of passed data. It performs strncmp, then simple_strtoul
and with this parameter calls mmc->block_dev.block_read(). 

But I'm a bit concern if adding function:

do_mmcops_check(unsigned int lba_start, unsigned int lba_end, ...) to

do_mmcops(argc, argv) {
	int i = simple_strtol(argv[]);
	return do_mmcops_check(i);
}

will help with preventing errors.

As I've written previously, data from prompt is passed in a form of
text, which is converted to the well defined type anyway (with e.g.
simple_strtoul - returns unsigned long, strict_strtoul - returns int,
simple_strtol - returns long, simple_strtoull - returns unsigned long
long). Using those functions correctly would ensure correct types
passed to e.g. mmc->block_dev.block_read().

When one create the do_mmcops_check() function, the compiler would
check if its arguments are correct (i.e. the i in the above example is
int). What is the difference between checking at compile time the
output of simple_strtoul? 

The real problem in my opinion is the lack of checking if arguments
passed as text to the do_mmcops are correct or not. This is not done
for MMC and I doubt, if adding compile time type checking (in a form of
a separate function) would solve/alleviate the problem.



> So, lets do
> what Marek was suggesting of making common/cmd_mmc.c and
> common/cmd_fat.c call a sub-function that takes compile-time
> typecheckable inputs, and call that here.  That opens things up for
> later making the user commands perform better checking and so forth.

I'd like to point to the problem with passing and then parsing
parameters as text. 

User typed parameters aren't checked.

Please correct me if I misunderstood the problem or the proposed
solution.

-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group