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

[Heinrich Schuchardt]

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
>
>
>