All of lore.kernel.org
 help / color / mirror / Atom feed
From: Heinrich Schuchardt <xypron.glpk@gmx.de>
To: u-boot@lists.denx.de
Subject: [PATCH 2/6] efi_loader: Add device path related functions for initrd via Boot####
Date: Thu, 11 Mar 2021 13:31:41 +0100	[thread overview]
Message-ID: <09eb1814-05fe-2b97-af85-a6843082cac7@gmx.de> (raw)
In-Reply-To: <70AA1ED8-9F73-4D50-891A-1FCB855F7094@gmx.de>

On 11.03.21 12:44, Heinrich Schuchardt wrote:
> Am 11. M?rz 2021 12:36:04 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>:
>> Hi Heinrich
>>
>> [...]
>>
>>>>>> + * @load_option: device paths to search
>>>>>> + * @size:	 size of the discovered device path
>>>>>> + * @guid:	 guid to search for
>>>>>> + *
>>>>>> + * Return:	device path or NULL. Caller must free the returned
>> value
>>>>>
>>>>> Please, keep the text aligned.
>>>>>
>>>>> Do we need a copy? Isn't a pointer good enough?
>>>>
>>>> A pointer to what?
>>>> I think it's better to a copy here. This is a device path that
>> might be used
>>>> out of a stack context were the load option might disappear.
>>>> Look at how the function is used in efi_get_dp_from_boot().
>>>
>>> You are duplicating in get_initrd_fp(). Why should we duplicate
>> twice?
>>>
>>
>> That's irrelevant though isn't it?
>> I did that in the efi initrd implementation. If someone else does the
>> DTB in
>> the future and device to use efi_dp_from_lo return directly?
>> I'd much prefer an API (since that function goes into an API-related
>> file for
>> device paths), that's safe and requires the user to free the memory,
>> rather
>> than allowing him to accidentally shoot himself in the foot, keeping in
>> mind
>> it's a single copy on a device path, which roughly adds anything on our
>> boot
>> time.
>>
>>>>
>>>>>
>>>>>> + */
>>>>>> +struct
>>>>>> +efi_device_path *efi_dp_from_lo(struct efi_load_option *lo,
>>>>>> +				efi_uintn_t *size, efi_guid_t guid)
>>>>>> +{
>>>>>> +	struct efi_device_path *fp = lo->file_path;
>>>>>> +	struct efi_device_path_vendor *vendor;
>>>>>> +	int lo_len = lo->file_path_length;
>>>>>> +
>>>>>> +	while (lo_len) {
>>>>>
>>>>> lo_len must be at least sizeof(struct efi_device_path).
>>>>>
>>>>>> +		if (fp->type != DEVICE_PATH_TYPE_MEDIA_DEVICE ||
>>>>>> +		    fp->sub_type != DEVICE_PATH_SUB_TYPE_VENDOR_PATH) {
>>>>>> +			lo_len -= fp->length;
>>>>>
>>>>> Could the last device path in the array be followed by zero bytes
>> for
>>>>> padding?
>>>>
>>>> How? Device paths are packed aren't they ?
>>>>
>>>>> Should we check that fp->length >= sizeof(struct efi_device_path)?
>>>>
>>>> Yea probably a good idea
>>>
>>> The content of the boot option comes from the user. Just assume that
>> it
>>> can contain malicious content.
>>>
>>
>> Yea the user doesn't add the device path directly though. The user adds
>> directories and a file, so the normalization is part of this function,
>> applied randomly and locally on a single input? or the device path
>> creation
>> functions which this code uses? Since we use the pattern in a bunch of
>> places
>> I assumed we did take care of that during the functions that create the
>> device
>> paths. I haven't checked though ...
>
> I am not referring to efidebug.
>
> The user can update EFI variables with any binary content using an EFI binary or OS functions.
>
> E.g. copy a binary file to the efi variables file system in Linux.
>
>>
>>> We should also check that the identified device-path starting at
>>> VenMedia() ends within fp->length using efi_dp_check_length().
>>
>> ok
>>
>>>
>>>>
>>>>>
>>>>>> +			fp = (void *)fp + fp->length;
>>>>>
>>>>> Please, avoid code duplication.
>>>>>
>>>>> E.g.
>>>>>
>>>>> for (; lo_len >= sizeof(struct efi_device_path);
>>>>>      lo_len -= fp->length, fp = (void *)fp + fp->length) {
>>>>
>>>> I can an switch to that, but I really never liked this format.
>>>> It always seemed way less readable to me for some reason.  Maybe
>> because I
>>>> never got used to it ...
>>>
>>> Using "for" is only one option. You could use "goto next;" instead.
>>>
>>
>> I really don't mind, I can just use what you propose.
>
> As long as you avoid code duplication I am fine.
>
> Best regards
>
> Heinrich
>
>>
>>>>
>>>>>
>>>>>> +			continue;
>>>>>> +		}
>>>>>> +
>>>>>> +		vendor = (struct efi_device_path_vendor *)fp;
>>>>>> +		if (!guidcmp(&vendor->guid, &guid))
>>>>>> +			return efi_dp_dup(fp);
>>>>>
>>>>> Should we strip of the VenMedia() node here?
>>>>
>>>> Why? This is not supposed to get the file path. The function says
>> "get device
>>>> path from load option" and that device includes the VenMedia node.
>>>> It would make more sense for me to strip in efi_get_dp_from_boot()
>> for
>>>> example, if you want a helper function to get the initrd path
>> *only*.
>>>
>>> The VenMedia() node is not needed anymore once you have found the
>> entry.
>>>
>>
>> Yea it's not but as I said the name of the function says "get the
>> *stored*
>>from a boot option. Not get the one that suits us.
>> There's another reason for that btw, the initrd related functions use
>> that
>> (specifically get_initrd_fp()), to figure out if the Boot#### variable
>> contains an initrd path or not.
>> If the VenMedia is not present at all, the protocol is not installed
>> allowing
>> the kernel to fallback in it's command line 'initrd=' option.
>> If the VenMedia is there though, we are checking the file path of the
>> initrd
>> and if the file's not found we return an error allowing Bootmgr to
>> fallback.
>>
>> If we 'just' return the initrd path, we'll have to introduce another
>> variable
>> in the function, indicating if the VenMedia is present or not so the
>> rest
>> ofthe codepath can decide what to do.
>>
>>>>
>>>> But really it's just one invocation of efi_dp_get_next_instance()
>> after
>>>> whatever device path you get. Which also modifies the device path
>> pointer, so
>>>> I'd really prefer keeping that call in a local context.
>>>
>>> Why next instance? I thought next node.
>>>
>>> My understanding is that we have:
>>>
>>> kernel path,end(0xff),
>>> VenMedia(), /* no end node here */
>>> initrd1, end(0x01),
>>> initrd2, end(0xff)
>>
>> No, the structure is added in cmd/efidebug.c code.
>> It's created with efi_dp_append_instance() on
>> - const struct efi_initrd_dp id_dp
>> - file path of initrd
>>
>> which will create:
>> kernel path,end(0xff),
>> VenMedia(), end(0x01),

This end node is superfluous.

Best regards

Heinrich

>> initrd1, end(0x01),
>> initrd2, end(0xff)
>>
>> I know I originally proposed the one you have, but it seemed cleaner
>> adding
>> an extra instance between VenMedia and the first initrd.
>>
>>>
>>> Please, document the structure.
>>>
>>
>> Sure
>>
>>> Best regards
>>>
>>> Heinrich
>>
>> Thanks
>> /Ilias
>

  reply	other threads:[~2021-03-11 12:31 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-05 22:22 [PATCH 1/6] efi_selftest: Remove loadfile2 for initrd selftests Ilias Apalodimas
2021-03-05 22:22 ` [PATCH 2/6] efi_loader: Add device path related functions for initrd via Boot#### Ilias Apalodimas
2021-03-11  7:50   ` Heinrich Schuchardt
2021-03-11  9:10     ` Ilias Apalodimas
2021-03-11 11:00       ` Heinrich Schuchardt
2021-03-11 11:36         ` Ilias Apalodimas
2021-03-11 11:44           ` Heinrich Schuchardt
2021-03-11 12:31             ` Heinrich Schuchardt [this message]
2021-03-11 12:39               ` Ilias Apalodimas
2021-03-11 12:44                 ` Heinrich Schuchardt
2021-03-11 12:49                   ` Ilias Apalodimas
2021-03-11 13:31             ` Ilias Apalodimas
2021-03-11 20:25               ` Heinrich Schuchardt
2021-03-12  2:50           ` AKASHI Takahiro
2021-03-12  4:10             ` Ilias Apalodimas
2021-03-12  4:32               ` AKASHI Takahiro
2021-03-12  4:42                 ` Ilias Apalodimas
2021-03-12  5:02                   ` AKASHI Takahiro
2021-03-12  5:19                     ` Ilias Apalodimas
2021-03-05 22:22 ` [PATCH 3/6] efi_loader: Introduce helper functions for EFI Ilias Apalodimas
2021-03-11  9:15   ` Heinrich Schuchardt
2021-03-05 22:23 ` [PATCH 4/6] efi_loader: Replace config option for initrd loading Ilias Apalodimas
2021-03-11 12:23   ` Heinrich Schuchardt
2021-03-11 12:30     ` Ilias Apalodimas
2021-03-11 12:50       ` Heinrich Schuchardt
2021-03-05 22:23 ` [PATCH 5/6] efidebug: add multiple device path instances on Boot#### Ilias Apalodimas
2021-03-11 12:38   ` Heinrich Schuchardt
2021-03-11 12:42     ` Ilias Apalodimas
2021-03-12  4:44   ` AKASHI Takahiro
2021-03-12  4:55     ` Ilias Apalodimas
2021-03-12  5:23       ` AKASHI Takahiro
2021-03-12  5:37         ` Ilias Apalodimas
2021-03-12  5:58           ` AKASHI Takahiro
2021-03-12  7:19             ` Ilias Apalodimas
2021-03-12 16:25     ` Heinrich Schuchardt
2021-03-05 22:23 ` [PATCH 6/6] doc: Update uefi documentation for initrd loading options Ilias Apalodimas
2021-03-11 12:39   ` Heinrich Schuchardt
2021-03-11  7:26 ` [PATCH 1/6] efi_selftest: Remove loadfile2 for initrd selftests Heinrich Schuchardt
2021-03-13 21:47 [PATCH 0/6 v2] Loadfile2 for initrd loading Ilias Apalodimas
2021-03-13 21:47 ` [PATCH 2/6] efi_loader: Add device path related functions for initrd via Boot#### Ilias Apalodimas
2021-03-14  7:19   ` Heinrich Schuchardt
2021-03-14  7:32     ` Ilias Apalodimas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=09eb1814-05fe-2b97-af85-a6843082cac7@gmx.de \
    --to=xypron.glpk@gmx.de \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.