[U-Boot] [RFC 1/1] cmd: fs: fix data abort in load cmd
Igor Opaniuk
igor.opaniuk at toradex.com
Fri Apr 12 11:18:48 UTC 2019
Thanks Heinrich!
Will test it ASAP.
On Wed, Apr 10, 2019 at 8:36 PM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 4/10/19 8:48 AM, Igor Opaniuk wrote:
> > 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.
>
> In
> http://git.denx.de/?p=u-boot-efi.git;a=shortlog;h=refs/heads/efi-2019-07
> I have already added a patch the problem with efi_dp_size().
>
> efi_loader: move efi_save_gd() call to board_r.c
> http://git.denx.de/?p=u-boot-efi.git;a=commit;h=5658c83d1c3637d4aa5bc366ab987b776ea105eb
>
> So I suggest that you start from the efi-2019-07 branch.
>
> Regards
>
> Heinrich
>
> >
> >> 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
> >
> >
> >
>
> _______________________________________________
> 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)
More information about the U-Boot
mailing list