Re: [RFC,PATCH] efi: Add support for a UEFI variable filesystem

Re: [RFC,PATCH] efi: Add support for a UEFI variable filesystem

[H. Peter Anvin]

On 08/30/2012 02:30 AM, Jeremy Kerr wrote:
> From: Matthew Garrett <mjg@redhat.com>
> 
> The existing EFI variables code only supports variables of up to 1024
> bytes. This limitation existed in version 0.99 of the EFI specification,
> but was removed before any full releases. Since variables can now be
> larger than a single page, sysfs isn't the best interface for this. So,
> instead, let's add a filesystem. Variables can be read, written and
> created, with the first 4 bytes of each variable representing its UEFI
> attributes. The create() method doesn't actually commit to flash since
> zero-length variables can't exist per-spec.
> 
> Updates from Jeremy Kerr <jeremy.kerr@canonical.com>.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> 
> ---
> RFC: With this patch, I can properly mange UEFI key databases from
> userspace. Comments welcome.
> 

Nice!

However, I have a question... rather than putting the attributes as the
first data bytes, would it be better to make it either part of the
filename (assuming there is at least one character other than / which
can be reasonably relied upon to not be part of the name); for example:

	LangCodes,BS,RT

... or ...

	LangCodes,6

	-hpa

Re: [RFC,PATCH] efi: Add support for a UEFI variable filesystem

[Jeremy Kerr]

Hi hpa,

Thanks for the review!

> However, I have a question... rather than putting the attributes as the
> first data bytes, would it be better to make it either part of the
> filename (assuming there is at least one character other than / which
> can be reasonably relied upon to not be part of the name); for example:
>
> 	LangCodes,BS,RT
>
> ... or ...
>
> 	LangCodes,6

This will get tricky when handling EFI_VARIABLE_APPEND_WRITE: this 
attribute will never appear in the attributes returned by GetVariable(), 
but may be passed to SetVariable(). If we put attributes in the 
filename, we'd need to handle writes to both names, and/or have 
duplicate dentries for each variable. We could do it, but the filesystem 
interface might be a little messy.

[Supporting append writes is essential for key database updates, which 
may be signed]

Cheers,


Jeremy

Re: [RFC,PATCH] efi: Add support for a UEFI variable filesystem

[H. Peter Anvin]

Wouldn't that be better handled by O_APPEND?



Jeremy Kerr <jeremy.kerr@canonical.com> wrote:

>Hi hpa,
>
>Thanks for the review!
>
>> However, I have a question... rather than putting the attributes as
>the
>> first data bytes, would it be better to make it either part of the
>> filename (assuming there is at least one character other than / which
>> can be reasonably relied upon to not be part of the name); for
>example:
>>
>> 	LangCodes,BS,RT
>>
>> ... or ...
>>
>> 	LangCodes,6
>
>This will get tricky when handling EFI_VARIABLE_APPEND_WRITE: this 
>attribute will never appear in the attributes returned by
>GetVariable(), 
>but may be passed to SetVariable(). If we put attributes in the 
>filename, we'd need to handle writes to both names, and/or have 
>duplicate dentries for each variable. We could do it, but the
>filesystem 
>interface might be a little messy.
>
>[Supporting append writes is essential for key database updates, which 
>may be signed]
>
>Cheers,
>
>
>Jeremy

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [RFC,PATCH] efi: Add support for a UEFI variable filesystem

[Jeremy Kerr]

hi hpa,

> Wouldn't that be better handled by O_APPEND?

Possibly, but this then means that there are now two "interfaces" that
specify the variable attributes.

[Also, in that case we should support the same mechanism through open();
llseek(0, SEEK_END) then, right?]

In general, I think the attributes-in-a-header mechanism is a little
tidier than providing them in the filename. For instance, finding if a
variable exists from userspace will require iterating the dentries (or
trying all combinations of variables), if the attributes aren't known.

However, I'm happy to implement this if it's the generally-preferred
solution.

Cheers,


Jeremy

Re: [RFC,PATCH] efi: Add support for a UEFI variable filesystem

[H. Peter Anvin]

Well, appending is an action, not really a property of the variable that sticks around, no?

Jeremy Kerr <jeremy.kerr@canonical.com> wrote:

>hi hpa,
>
>> Wouldn't that be better handled by O_APPEND?
>
>Possibly, but this then means that there are now two "interfaces" that
>specify the variable attributes.
>
>[Also, in that case we should support the same mechanism through
>open();
>llseek(0, SEEK_END) then, right?]
>
>In general, I think the attributes-in-a-header mechanism is a little
>tidier than providing them in the filename. For instance, finding if a
>variable exists from userspace will require iterating the dentries (or
>trying all combinations of variables), if the attributes aren't known.
>
>However, I'm happy to implement this if it's the generally-preferred
>solution.
>
>Cheers,
>
>
>Jeremy

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [RFC,PATCH] efi: Add support for a UEFI variable filesystem

[Jeremy Kerr]

Hi hpa,

> Well, appending is an action, not really a property of the variable
> that sticks around, no?

True, but they're still all defined as the same thing in the UEFI spec.
If you're looking to define which attributes to pass, you now need to
know the extra information that you pass most of the SetVariable()
attributes through the filename, but APPEND_WRITE is passed a completely
different way.

To me, this sounds fairly non-intuitive for a developer to discover.

Cheers,


Jeremy

Re: [RFC,PATCH] efi: Add support for a UEFI variable filesystem

[Matthew Garrett]

On Sun, Sep 02, 2012 at 06:59:37PM -0700, H. Peter Anvin wrote:
> Well, appending is an action, not really a property of the variable that sticks around, no?

We could do this for append, but what happens if an attribute with 
similar variables appears and doesn't neatly map to an O_ option? We 
can't really argue for limiting attributes purely because they don't map 
neatly onto POSIX semantics...

-- 
Matthew Garrett | mjg59@srcf.ucam.org