efirt: add ESRT table support

[pavel.balaev_3mdeb.com]

Summary

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.

[pavel.balaev_3mdeb.com]

sys/dev/efidev/efirt.c

396

EFI_PROPERTIES_TABLE_GUID will be simpler.

[kib]

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.

[pavel.balaev_3mdeb.com]

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;

[kib]

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.

[pavel.balaev_3mdeb.com]

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;

[pavel.balaev_3mdeb.com]

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

[pavel.balaev_3mdeb.com]

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

[kib]

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?

[pavel.balaev_3mdeb.com]

sys/dev/efidev/efirt.c

396

phys address of esrt table is: 0xac25d598.

[pavel.balaev_3mdeb.com]

Patch was reworked

[pavel.balaev_3mdeb.com]

sys/dev/efidev/efirt.c

396

new patch pushed

[kib]

So does this work now?

Please upload the patch with the context (diff -U9999999)

[pavel.balaev_3mdeb.com]

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.

[kib]

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

37

Please order includes alphabetically. sys/uio.h goes right before sys/vmmeter.h

43

malloc.h is before module.h

64

we have nitems() macro, please use it and not this hand-made one

353

Unindent this '{' by one tab and correspondingly all lines under its scope.

373

Try to add D30785 to your test setup. I think this is the problem.

[pavel.balaev_3mdeb.com]

sys/dev/efidev/efirt.c

373

Thanks, this this patch all works as expected!

[kib]

sys/dev/efidev/efirt.c

373

Ok, the physcopyin() fix landed into HEAD. You can finish polishing the patch.

[kib]

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
.Dv NULL
.Va table_len
.Va buf_len

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

2 ↗

(On Diff #91103)

Any contact information like email?

32 ↗

(On Diff #91103)

Might be, not interaction, but dumping? At least with the current functionality, this is more descriptive.

46 ↗

(On Diff #91103)

New line for each sentence.

usr.sbin/efitable/efitable.c

37 ↗

(On Diff #91103)

Order headers:
sys/ headers first, then userspace headers.
In each block, order headers alphabetically.
(Except when dependencies require other ordering, perhaps you need to include sys/types.h first)

43 ↗

(On Diff #91103)

Again, use nitems from sys/param.h

54 ↗

(On Diff #91103)

static const?

67 ↗

(On Diff #91103)

Style does not allow for empty lines in declaration block.

72 ↗

(On Diff #91103)

Why not use bools for types there? You would need stdbool.h but that's it.

73 ↗

(On Diff #91103)

Same

136 ↗

(On Diff #91103)

Consider using err(3) helpers.

171 ↗

(On Diff #91103)

static void
efi_table_print_esrt(....

228 ↗

(On Diff #91103)

Are you aware of libxo(3)?

246 ↗

(On Diff #91103)

Same formatting nit.

248 ↗

(On Diff #91103)

Why do you need to initialize it with NULL? You assign to the var on the next line.

256 ↗

(On Diff #91103)

+ 4 spaces indent.

[pavel.balaev_3mdeb.com]

usr.sbin/efitable/efitable.c

228 ↗

(On Diff #91103)

Will use it, thanks!

[pavel.balaev_3mdeb.com]

All problems was fixed

[kib]

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

375

Indent should be +4 spaces for the continuation line.

376

Should do free(buf) there?

380

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.

410

Again, add some arbitrary limit?

usr.sbin/efitable/Makefile.depend

2 ↗

(On Diff #91236)

I am not sure that you need this file

[emaste]

share/man/man4/efidev.4

74–89

I would probably write this as "Copy the UEFI table specified by the uuid field of struct ...."

81

maybe s/found/queried/

usr.sbin/efitable/efitable.8

32 ↗

(On Diff #91236)

Dump UEFI tables

54–55 ↗

(On Diff #91236)

This should be a mdoc list or table, probably table name / description.

[pavel.balaev_3mdeb.com]

Fix man pages and padding

This revision is broken, extra files pulled

[pavel.balaev_3mdeb.com]

patch without unnecessary files

[kib]

Please provide the line for git commit author's field.

sys/dev/efidev/efirt.c

384

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.

[pavel.balaev_3mdeb.com]

sys/dev/efidev/efirt.c

384

OK, so I can only use arbitrary limits without overflow check, or use limit with different overflow check mechanism?

[kib]

sys/dev/efidev/efirt.c

384

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 */
}

[pavel.balaev_3mdeb.com]

In D30104#696601, @kib wrote:

Please provide the line for git commit author's field.

I added fit commit author but this review eats it O don't know why

[pavel.balaev_3mdeb.com]

fix overflow another way

[pavel.balaev_3mdeb.com]

In D30104#697064, @pavel.balaev_3mdeb.com wrote:

fix overflow another way

This patch was made from git

git format-patch -s -1 -U9999999

[kib]

Can you put the author line as a comment into phab? Phab reformats diffs, effectively stripping anything not being the diff.

[pavel.balaev_3mdeb.com]

Author: Pavel Balaev <[e-mail]>