This patch enables copying UEFI tables to user space.
EFIIOC_GET_TABLE ioctl was resurrected and reworked.
Added new userspace tool called efitable.
This is useful for firmware update tools such as fwupd.
Details
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
sys/dev/efidev/efirt.c | ||
---|---|---|
396 | EFI_PROPERTIES_TABLE_GUID will be simpler. |
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() ? and physcopyout(vm_paddr_t src, void *dst, size_t len) |
sys/dev/efidev/efirt.c | ||
---|---|---|
396 | after debugging physcopyout((vm_paddr_t)esrt, buf, len); 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. |
sys/dev/efidev/efirt.c | ||
---|---|---|
396 | Can you push the current patch as is (non-working)? I will read it as a whole. 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? |
sys/dev/efidev/efirt.c | ||
---|---|---|
396 | phys address of esrt table is: 0xac25d598. |
sys/dev/efidev/efirt.c | ||
---|---|---|
396 | new patch pushed |
Patch with context.
Still have a problem:
get_table_length: esrt:0xac25d598, buf=0xfffff80002834750, len=16
uiomove_fromphys:bcopy(0xfffff80000000598, 0xfffff80002834750, 16)
get_table_length fw_resource_count: 0 (must be 1 on my hardware)
ESRT phys addr is: 0xac25d598, with efi_enter() works fine.
share/man/man4/efidev.4 | ||
---|---|---|
29 | The date of the man page should be updated | |
97 | This is too terse. Please explain what table_len and buf_len are for, which are in and which are out, and how to use them. | |
sys/dev/efidev/efidev.c | ||
62 | extra (), this should be written as error = efi_copy_table(&egtioc->uuid, egtioc->buf != NULL ? &buf : NULL, | |
69 | This is probably too strict, why not buf_len >= table_len? | |
sys/dev/efidev/efirt.c | ||
38 | Please order includes alphabetically. sys/uio.h goes right before sys/vmmeter.h | |
44 | malloc.h is before module.h | |
65 | we have nitems() macro, please use it and not this hand-made one | |
354 | Unindent this '{' by one tab and correspondingly all lines under its scope. | |
374 | Try to add D30785 to your test setup. I think this is the problem. |
sys/dev/efidev/efirt.c | ||
---|---|---|
374 | Thanks, this this patch all works as expected! |
sys/dev/efidev/efirt.c | ||
---|---|---|
374 | Ok, the physcopyin() fix landed into HEAD. You can finish polishing the patch. |
Please upload the diff with context.
(this is probably out of scope for current review, lets finish it first) I wondering if a useful addition to the interface would be to report just the address of the table for unknown UUIDs. Then user or even efitable can dump a user-defined number of bytes.
share/man/man4/efidev.4 | ||
---|---|---|
76 | Each sentence should start at the new line. .Va buf | |
sys/dev/efidev/efidev.c | ||
62 | ...->buf != NULL ? ... | |
63 | Indent for continuation line should be +4 spaces. | |
sys/dev/efidev/efirt.c | ||
357 | Style discourages initialization in declaration, and we certainly do not do complex initialization (like calling malloc(9)) in declaration. | |
362 | This break results in returning ENOENT on case of efi_get_table() error. I do not think it is right, the original error should be returned. | |
368 | Same note about error. | |
373 | indent | |
374 | AFAIR ENXIO is also returned if EFI RT services are not available. I do not think that userspace can practically distinguish between cases, perhaps a different error value would be more useful. | |
428 | static const struct ... BTW if you insist on defining known_table scoped in the function, you can do something like static const struct known_table{...} tables[] = {...}; | |
444 | Remove table_supported variable, and change the condition to table_idx == nitems(tables). | |
usr.sbin/efitable/efitable.8 | ||
3 | Any contact information like email? | |
33 | Might be, not interaction, but dumping? At least with the current functionality, this is more descriptive. | |
47 | New line for each sentence. | |
usr.sbin/efitable/efitable.c | ||
38 | Order headers: | |
44 | Again, use nitems from sys/param.h | |
55 | static const? | |
68 | Style does not allow for empty lines in declaration block. | |
73 | Why not use bools for types there? You would need stdbool.h but that's it. | |
74 | Same | |
137 | Consider using err(3) helpers. | |
172 | static void efi_table_print_esrt(.... | |
229 | Are you aware of libxo(3)? | |
247 | Same formatting nit. | |
249 | Why do you need to initialize it with NULL? You assign to the var on the next line. | |
257 | + 4 spaces indent. |
usr.sbin/efitable/efitable.c | ||
---|---|---|
229 | Will use it, thanks! |
Please provide me either with:
- your full email address in form "FirstName LastName <your@email>"
- or git format-patch output for the branch, with author metadata correctly filled, and with the complete commit message
Thank.
sys/dev/efidev/efirt.c | ||
---|---|---|
376 | Indent should be +4 spaces for the continuation line. | |
377 | Should do free(buf) there? | |
381 | In principle, it would be nice to check the mul operation for overflow. Also in principle, it would be nice to put some arbitrary limit there, so that we do not try to malloc() excessively large kernel memory later, kernel really does not tolerate such attemtps. This is mostly to have some minimal protection against buggy bioses. | |
411 | Again, add some arbitrary limit? | |
usr.sbin/efitable/Makefile.depend | ||
2 ↗ | (On Diff #91236) | I am not sure that you need this file |
Please provide the line for git commit author's field.
sys/dev/efidev/efirt.c | ||
---|---|---|
385 | No, please do not bring linuxkpi to native freebsd code. Checking for overflow is good but not enough, kernel cannot handle even much smaller sizes for overgrown alloc requests. Put some reasonable limit there, say 128K sounds good and arbitrary. If you worry that it might be too limiting, add sysctl that manages the limit. |
sys/dev/efidev/efirt.c | ||
---|---|---|
385 | OK, so I can only use arbitrary limits without overflow check, or use limit with different overflow check mechanism? |
sys/dev/efidev/efirt.c | ||
---|---|---|
385 | It is easy to combine overflow and limit checks into one condition, using standard C: #define EFI_TABLE_ALLOC_MAX (128 * 1024 * 1024) if (fw_resource_count > EFI_TABLE_ALLOC_MAX / sizeof(struct efi_esrt_entry_v1)) { free(buf, M_TEMP); return (ENODEV); /* Not sure that this error code is best, but ok */ } |
Can you put the author line as a comment into phab? Phab reformats diffs, effectively stripping anything not being the diff.