Page MenuHomeFreeBSD

efirt: add ESRT table support
Needs ReviewPublic

Authored by pavel.balaev_3mdeb.com on May 4 2021, 6:01 PM.

Details

Reviewers
kib
kevans
Group Reviewers
Contributor Reviews (base)
Summary

This patch enables publishing ESRT table to user space.
This is usefull for firmware update tools such as fwupd.

ESRT table represent via read-only sysctl variables.
User can disable ESRT representing via variable
"efi.esrt.disable=1" in loader.conf.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Please generate diffs with '-U9999999' option, so that the full context is available.

stand/man/loader.8
564

Why do you think this control is needed? What is the scenario where the export would give some trouble?

sys/dev/efidev/efirt.c
377

These are structures definitions as provided by UEFI spec, right?
I suggest to put them e.g. into sys/efi.h, but see another big question below, answer to which might affect this.

387

This should be like this
esrt_ptr = (struct esrt_table *)efi_phys_to_kv((vm_paddr_t)ptr);

390

if (esrt_ptr == NULL || ...

396

So the big question:
why did you decided to parse the table in kernel, and provided the parsed representation from sysctl? Look, for instance, at our machdep.efi_map. We push out the blob of the table, and e.g. sysctl(8) pretty-prints elements.

Should we do the same there?

kib removed a subscriber: kib.
stand/man/loader.8
564

I don't think so, I agree, this is unnecessary

sys/dev/efidev/efirt.c
377

Yes, from spec. Got it.

387

ok.

396

OK, this is good idea. I will rework the patch. Resubmit it in this review, or create new?

sys/dev/efidev/efirt.c
396

Better to continue with this review, so that the whole history of the patch is kept in single place.

pavel.balaev_3mdeb.com added inline comments.
sys/dev/efidev/efirt.c
396

Do you mean that we should parse ESRT table from user-space tool?
F.e. Linux parse it in kernel and provide sysfs representation: ()https://github.com/torvalds/linux/blob/master/drivers/firmware/efi/esrt.c)
So this patch do the same, but use sysctl variables.
Is my solution is so bad?

sys/dev/efidev/efirt.c
396

What is the advantage of parsing this table in kernel? Do you (or somebody else) plan to have a consumer for this table in kernel?

If not, then why adding more code to kernel, which should deal with not very trusted source?

If there are uses of it in kernel, we should think how to represent it internally, and only then select the external representation. Per-entry sysctl is really not how sysctls should be used.

sys/dev/efidev/efirt.c
396

No consumers in kernel planned. So I agree that my solution is wrong.
So I need to push blob, containing ESRT data (do not know how to do it now, but will look at machdep.efi_map) and will parse it in sysctl:

else if (strcmp(fmt, "S,efi_esrt) == 0)

func = S_efi_esrt;

something like this?

sys/dev/efidev/efirt.c
396

I am not even sure that sysctl(2)/sysctl(8) is the right tool to use there. I just had a flash-back.

We have efidev(4) AKA /dev/efi, which provided (broken) EFIIOC_GET_TABLE ioctl. I think the right route there is to resurrect the ioctl, perhaps fixing it e.g. to actually work and eliminate the need to use KVA access. For instance, it might take the pointer to userspace buffer where to copy-out the requested EFI table.

Then we need a userspace tool that would use the ioctl to fetch some table, parse it, and provide the usable representation of data e.g. for scripts and for humans. I do not think that we have a tool that can be logically extended, so a new utility might be a reasonable proposition.

sys/dev/efidev/efirt.c
396

OK got it, but have one question:
I don't get how to eliminate KVA access, so for example userspace invokes ioctl and request ESRT (or some else) table. In kernel space I can get phys addr of table ( efi_get_table() ), but without phys_to_kva I cannot even copy data to buffer, also I don't know the size of table data.

sys/dev/efidev/efirt.c
396

I can phys_to_kva, then parse data in kernel space into buffer, then copy it to user. This is simple, but I think it is wrong.

sys/dev/efidev/efirt.c
396

Got it, I can just use cast (vm_paddr_t)ptr, I will resurrect this syscall, and see why it not works and try to fix it.

sys/dev/efidev/efirt.c
396

Sorry I was not clear enough. To see what I mean by 'eliminating access to KVA' you need to see the original (removed) ioctl GET_TABLE interface. It returned some address for the table to userspace, and then userspace needs to access /dev/mem to fetch the actual table data.

Sane ioctl interface would take a pointer to userspace buffer and copyout the table to buffer, eliminating any /dev/mem activities. Sure, the kernel side would need to allocate some bounce buffer, enter EFIRT mapping mode, copy to the buffer, then exit EFIRT and copyout the buffer to userspace.

sys/dev/efidev/efirt.c
396

Thanks, will try to create sane ioctl interface.

sys/dev/efidev/efirt.c
396

enter EFIRT mapping mode

Do you mean efi_enter()/efi_leave(), and is a any way to get partition table size?

sys/dev/efidev/efirt.c
396

I mean that we can access to table data:
efi_enter();
struct efi_cfgtbl *ct;
ct->ct_data;
efi_leave();
But we do not know the length of it. So we need know partition size to copy data to buffer,

sys/dev/efidev/efirt.c
396

I do not quite understand what do you mean by 'partition size'. I will answer two other questions instead, in the hope that one of them is actually what you intended to ask.

First, userspace ioctl interface should provide the size of the incoming buffer for kernel to copy out the table. Userspace cannot know in advance how large the buffer should be. Typical interface is designed to return the actual copied length alongside the data, but passing a NULL pointer for the buffer results in returning the total length needed. See for instance sysctl(2). So if, for instance, ioctl takes the following structure

struct efi_esrt_io {
   void *buf; /* in, pointer to buffer */
   size_t buf_len; /* in, size of the buffer */
   size_t filled_len; /* out, how much of the buffer is filled */
};

and setting buf = NULL would provide the filled_len of the required buf size.

Second, the length of the table is stored in the table itself, you need to copy FwResourceCount EFI_SYSTEM_RESOURCE_ENTRYies. Yes, you can read the counter after efi_enter(), then do efi_leave(), allocate the memory, then do efi_enter() again and copy to that temporal buffer, then efi_leave() again. Now you can copy out to userspace and free the buffer.

sys/dev/efidev/efirt.c
396

Thanks for the answer, about userspace ioctl interface it's all clear.
I mean that if I get length of data with FwResourceCount and EFI_SYSTEM_RESOURCE_ENTRY, this would be valid only for ESRT table, not the others
So if we resurrect this ioctl we need different methods get_length for all known UUID?
If it is OK my implementation will only support ESRT table, in future we can expand supported table list

The last question is: can I dereference struct efi_cfgtbl *ct; ct->ct_data after efi_enter() without using efi_phys_to_kva() ?

sys/dev/efidev/efirt.c
396

Yes, and yes. Your implementation should be aware of the table' type and the way to calculate the length for specific table type.

I would be more certain about some design decisions and interfaces if you handle ESRT and one more table, does not matter which. Only to avoid making the generalization from one data point.

sys/dev/efidev/efirt.c
396

Thanks! I will try to find one more sutable table to add.

sys/dev/efidev/efirt.c
396

I looked at the EFI spec 4.6 EFI_CONFIGURATION_TABLE definition, and I see that they require void *VendorTable to be the physical address, not EFIRT remapped address. So you do not need efi_enter()/leave() at all and can directly access the physical addresses instead. uiomove_fromphys(9) should do it.

sys/dev/efidev/efirt.c
396

This is valid only for copying? for get_length() I should use efi_enter() anyway?

sys/dev/efidev/efirt.c
396

And second table will be EFI_DEBUG_IMAGE_INFO_TABLE. Is it OK?

sys/dev/efidev/efirt.c
396

EFI_PROPERTIES_TABLE_GUID will be simpler.

sys/dev/efidev/efirt.c
396

Formally efi_enter() is not valid, if we ever switch from 1:1 EFIRT memory mapping to something more involved. BTW, you can use uiomove_fromphys() with UIO_SYSSPACE to the header.

396

Ok, although not very useful.

sys/dev/efidev/efirt.c
396

I've got this results:

this works fine:

efi_get_table(&uuid, (void **)&esrt);
efi_enter();                                                                                              
fw_resource_count = esrt->fw_resource_count;                                                              
efi_leave();

panic: page fault:

efi_get_table(&uuid, (void **)&esrt);                                                                                       
fw_resource_count = esrt->fw_resource_count;

unexpected data: (maybe I use this function wrong)

void *buf = malloc(len, M_TEMP, M_WAITOK); 
efi_get_table(&uuid, (void **)&esrt);
physcopyout((vm_paddr_t)esrt, buf, len);
fw_resource_count = ((struct efi_esrt_table *)buf)->fw_resource_count;
sys/dev/efidev/efirt.c
396

Second snippet faulted because you tried to deferefence a pointer belonging to the physical address space. Kernel is executing in Kernel Virtual Address space, so it cannot do it this way.

Third snippedt is wrong because you did physcopyout() overwriting the table instead of physcopyin(), copying from table to your buffer. WIth that detail fixed, I hope this would work and this is the best way to get to the table.

sys/dev/efidev/efirt.c
396

Are you sure about about physcopyin() ?
it has input args: physcopyin(void *src, vm_paddr_t dst, size_t len)
and uio.uio_rw = UIO_WRITE;

and physcopyout(vm_paddr_t src, void *dst, size_t len)
has uio.uio_rw = UIO_READ;

sys/dev/efidev/efirt.c
396

after debugging physcopyout((vm_paddr_t)esrt, buf, len);
I see:
get_table_length: esrt:0xac25d598, buf=0xfffff80002b81950, len=16
uiomove_fromphys:bcopy(0xfffff80000000598, 0xfffff80002b81950, 16)

define bcopy(a,b,c) memmove(b,a,c)

so physcopyout() copy data from phys to iov_base but data is wrong

sys/dev/efidev/efirt.c
396

Even from userspace a can mmap /dev/mem with offset=0xac25d598 and get actual data.
I dont get where am I wrong

sys/dev/efidev/efirt.c
396

Can you push the current patch as is (non-working)? I will read it as a whole.
I think you are right about physcopyin, and I am wrong.

The bcopy line above looks somewhat stange. The source address for bcopy is 0xfffff80000000598. The direct map starts at 0xfffff80000000000, so basically you are reading from the physical address 0x598, which is unlikely to contain a UEFI table. What is the phys address for the ESRT table that is given to you?