[U-Boot] [RFC 1/1] cmd: fs: fix data abort in load cmd

[Igor Opaniuk]

Hi Heinrich,

Thanks for looking into this,

On Tue, Apr 9, 2019 at 11:28 PM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 4/9/19 3:08 PM, Igor Opaniuk wrote:
> > With CONFIG_CMD_BOOTEFI=y, load command causes data abort
> > when path_to_uefi(fp->str, path) tries to write uefi path out of
> > bounds of u16 str[] array (check efi_device_path_file_path struct for
> > details). This is caused by unproper handling of void *buf pointer
> > in efi_dp_from_file(), particularly when the buf pointer value is changed
> > after dp_part_fill() invocation.
> >
> >> load usb 0:1 0x12000000 imx6dl-colibri-eval-v3.dtb
> > pc : [<2fab48ae>]        lr : [<2fab4339>]
> > reloc pc : [<178338ae>]          lr : [<17833339>]
> > sp : 2da77120  ip : 00000003   fp : 00000005
> > r10: 2daa31d0  r9 : 2da80ea8   r8 : 00000001
> > r7 : 2daa3098  r6 : 2ca75040   r5 : 2da77148  r4 : 0000003a
> > r3 : 00000069  r2 : 2ca750a3   r1 : 2daa3104  r0 : 2ca7509f
> > Flags: nzCv  IRQs off  FIQs off  Mode SVC_32
> > Code: 4630fb31 81f0e8bd e7d84606 bf082b2f (f822235c)
> > Resetting CPU ...
> >
>
> Thanks for reporting the problem.
>
> > With the change suggested:
> >
> >> load usb 0:1 0x12000000 imx6dl-colibri-eval-v3.dtb
> > 5675440 bytes read in 188 ms (28.8 MiB/s)
> >
> > Signed-off-by: Igor Opaniuk <igor.opaniuk at toradex.com>
> > ---
> >  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 53b40c8c3c..97b4356167 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -829,7 +829,7 @@ struct efi_device_path *efi_dp_from_file(struct blk_desc *desc, int part,
> >               buf = dp_part_fill(buf, desc, part);
> >
> >       /* add file-path: */
> > -     fp = buf;
> > +     fp = start;
>
> This cannot be correct. dp_part_fill() is meant to set buf to the end of
> the partition device path, e.g.
> /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8)/HD(1,MBR,0xd1535d21,0x1,0x7f)

That's why I added RFC tag, as I definitely don't understand the full
picture of what's going here(and lack of time to discover) :)

>
> In the lines below we want to add a further device path node with the
> filename followed by the end node, e.g.
>
> /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8)/HD(1,MBR,0xd1535d21,0x1,0x7f)/Shell.efi
>
> With your patch we end up with a device path containing only the file
> name and the end node, e.g.
>
> /Shell.efi

Thanks for the explanation.

>
> If you think this is an out of bound problem we must fix the estimation
> of the device path size.
>
> For better understanding the problem could you, please, print the value
> of dpsize and then call dp_alloc() with a sufficiently large argument.
>
> Before the return statement add
>
> printf("desc %p\n", desc);
> printf("dp length %zu\n", efi_dp_size(start));
>
> This should provide the calculated device path length and its actual size.

So with the changes:

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 97b4356167..ef707e2505 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -821,6 +821,8 @@ struct efi_device_path *efi_dp_from_file(struct
blk_desc *desc, int part,
        fpsize = sizeof(struct efi_device_path) + 2 * (strlen(path) + 1);
        dpsize += fpsize;

+       printf("dpsize %zu\n", dpsize);
+
        start = buf = dp_alloc(dpsize + sizeof(END));
        if (!buf)
                return NULL;
@@ -837,6 +839,8 @@ struct efi_device_path *efi_dp_from_file(struct
blk_desc *desc, int part,
        buf += fpsize;

        *((struct efi_device_path *)buf) = END;
+       printf("desc %p\n", desc);
+       printf("dp length %zu\n", efi_dp_size(start));

        return start;
 }

Output:
dpsize 153
desc 2daa3d30

And U-boot hangs on efi_dp_size(start) invocation, so I never receive
any output from the last printf() invocation.

> Please, indicate the config file that you are using.

colibri_imx6_defconfig

>
> Best regards
>
> Heinrich
>
> >       fp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
> >       fp->dp.sub_type = DEVICE_PATH_SUB_TYPE_FILE_PATH;
> >       fp->dp.length = fpsize;
> >
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Senior Development Engineer,
Igor Opaniuk

Toradex AG
Altsagenstrasse 5 | 6048 Horw/Luzern | Switzerland | T: +41 41 500 48
00 (main line)