[U-Boot] [PATCH 2/3] efi: add protection for block_dev

[Patrick DELAUNAY]

Hi Heinrich

> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
> 
> On 4/10/19 11:02 AM, Patrick Delaunay wrote:
> > Check the value of block_dev before to use this pointer.
> >
> > This patch solves problem for the command "load" when ubifs is
> > previously mounted: in this case the function
> > blk_get_device_part_str("ubi 0") don't return error but return
> > block_dev = NULL and then data abort.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> > ---
> >
> > To reproduce the issue, I have a boot script 'boot.scr.uimg'
> > with a load command executed during ubi boot:
> >
> > load ${devtype} ${devnum}:${distro_bootpart} ${m4fw_addr} ${m4fw_name}
> >
> > I have a data abort for call stack:
> > - do_load_wrapper for "ubi 0"
> > -- efi_set_bootdev
> > --- efi_dp_from_name
> >
> > => desc = 0 and data abort for access to 'desc->*'
> 
> Thanks for reporting and analyzing the problem
> 
> Where exactly is the NULL dereference occurring?

I see the issue on v2018.11 and I adapt the patch for v2019.04 (as efi_dp_from_name as be created in v2019.01)

On v2018.11, the stack frame was

do_load_wrapper
=> efi_set_bootdev
==> efi_dp_from_name
===> efi_dp_from_part
====> dp_part_size

With result for blk_get_device_part_str
  dev="ubi"
  devnr="0:"
  path="/boot.scr.uimg"

desc =00000000 part=0

And the crash occurs in the function dp_part_size / called from efi_dp_from_part

In trace :
	pc : [<ffc91d28>]	   lr : [<ffc92383>]
	reloc pc : [<c0166d28>]	   lr : [<c0167383>]

with symbol 

	System.map:c0167358 T efi_dp_is_multi_instance
	System.map:c0167378 T efi_dp_from_part
	System.map:c01673a4 T efi_dp_part_node
	System.map:c01673c8 T efi_dp_from_file

	System.map:c0166d22 t dp_part_size
	System.map:c0166d50 t dp_part_node


> Igor reported a similar bug for a USB device in
> cmd: fs: fix data abort in load cmd
> https://lists.denx.de/pipermail/u-boot/2019-April/364484.htmll
> 
> >
> > I also proposed a protection for the same issue in ums command
> > http://patchwork.ozlabs.org/project/uboot/list/?series=68096
> >
> >
> >   lib/efi_loader/efi_device_path.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/efi_loader/efi_device_path.c
> > b/lib/efi_loader/efi_device_path.c
> > index 53b40c8..fd57be8 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -970,7 +970,7 @@ efi_status_t efi_dp_from_name(const char *dev, const
> char *devnr,
> >   	if (!is_net) {
> >   		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
> >   					       1);
> > -		if (part < 0)
> > +		if (part < 0 || !desc)
> 
> part = 0, desc = NULL occurs for UBI if the UBI file system is mounted.
> 
> Returning an error here means in the end that we will not be able to install run
> GRUB from the UBI device because we cannot describe the boot device.
> 
> I think that UBI volumes should be handled like any other block device.
> This will avoid having separate program paths for UBI and not UBI.
> 
> Heiko and Kyungmin could you, please, explain why UBI currently is not providing
> a struct blk_desc * block descriptor and how this can be fixed.
> 
> Best regards
> 
> Heinrich
> 
>  >   			return EFI_INVALID_PARAMETER;
> >
> >   		if (device)
> >

Best regards

Patrick