| Message ID | 20200514123831.30157-3-michael@walle.cc |
|---|---|
| State | New |
| Headers | show |
| Series | bootefi fixes for aarch64/layerscape | expand |
On 5/14/20 2:38 PM, Michael Walle wrote: > The first argument has to be aligned with EFI_PAGE_SIZE. This alignment > is already checked for external callers but it is not checked for > internal callers. Unfortunately, most of the time the return value is > not checked, so scream loud and clear. Why do you mention the return value here? > > Signed-off-by: Michael Walle <michael at walle.cc> > --- > lib/efi_loader/efi_memory.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c > index fd79178da9..b56e19cb30 100644 > --- a/lib/efi_loader/efi_memory.c > +++ b/lib/efi_loader/efi_memory.c > @@ -248,6 +248,9 @@ efi_status_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type, > EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, > start, pages, memory_type, overlap_only_ram ? "yes" : "no"); > > + if (start & EFI_PAGE_MASK) > + panic("%s: start not aligned\n", __func__); > + Did you find any internal caller that has a problem? We do not want to increase code size. Best regards Heinrich > if (memory_type >= EFI_MAX_MEMORY_TYPE) > return EFI_INVALID_PARAMETER; > >
Am 2020-05-14 20:35, schrieb Heinrich Schuchardt: > On 5/14/20 2:38 PM, Michael Walle wrote: >> The first argument has to be aligned with EFI_PAGE_SIZE. This >> alignment >> is already checked for external callers but it is not checked for >> internal callers. Unfortunately, most of the time the return value is >> not checked, so scream loud and clear. > > Why do you mention the return value here? most callers just ignore the return value. so if not aligned this will silently fail. >> >> Signed-off-by: Michael Walle <michael at walle.cc> >> --- >> lib/efi_loader/efi_memory.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >> index fd79178da9..b56e19cb30 100644 >> --- a/lib/efi_loader/efi_memory.c >> +++ b/lib/efi_loader/efi_memory.c >> @@ -248,6 +248,9 @@ efi_status_t efi_add_memory_map(uint64_t start, >> uint64_t pages, int memory_type, >> EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, >> start, pages, memory_type, overlap_only_ram ? "yes" : "no"); >> >> + if (start & EFI_PAGE_MASK) >> + panic("%s: start not aligned\n", __func__); >> + > > Did you find any internal caller that has a problem? See next patch. > We do not want to increase code size. Mh, even within the efi_loader? Well I could do a if (start & EFI_PAGE_MASK) { debug("%s: start not aligned\n", __func__); return EFI_INVALID_PARAMETER; } but as I said, nobody checks the return value.
On 5/14/20 8:50 PM, Michael Walle wrote: > Am 2020-05-14 20:35, schrieb Heinrich Schuchardt: >> On 5/14/20 2:38 PM, Michael Walle wrote: >>> The first argument has to be aligned with EFI_PAGE_SIZE. This alignment >>> is already checked for external callers but it is not checked for >>> internal callers. Unfortunately, most of the time the return value is >>> not checked, so scream loud and clear. >> >> Why do you mention the return value here? > > most callers just ignore the return value. so if not aligned this will > silently fail. > >>> >>> Signed-off-by: Michael Walle <michael at walle.cc> >>> --- >>> ?lib/efi_loader/efi_memory.c | 3 +++ >>> ?1 file changed, 3 insertions(+) >>> >>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >>> index fd79178da9..b56e19cb30 100644 >>> --- a/lib/efi_loader/efi_memory.c >>> +++ b/lib/efi_loader/efi_memory.c >>> @@ -248,6 +248,9 @@ efi_status_t efi_add_memory_map(uint64_t start, >>> uint64_t pages, int memory_type, >>> ???? EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, >>> ?????????? start, pages, memory_type, overlap_only_ram ? "yes" : "no"); >>> >>> +??? if (start & EFI_PAGE_MASK) >>> +??????? panic("%s: start not aligned\n", __func__); >>> + >> >> Did you find any internal caller that has a problem? > See next patch. > >> We do not want to increase code size. > Mh, even within the efi_loader? Well I could do a > > if (start & EFI_PAGE_MASK) { > ??? debug("%s: start not aligned\n", __func__); > ??? return EFI_INVALID_PARAMETER; > } > > but as I said, nobody checks the return value. The Linux kernel has this nice code comment: "Don't use BUG() or BUG_ON() unless there's really no way out;" Looking at the different internal callers, some of these actively ensure alignment others don't. Wouldn't it make more sense to move all rounding into a common function taking a start address and a size in bytes as arguments. Best regards Heinrich > >
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index fd79178da9..b56e19cb30 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -248,6 +248,9 @@ efi_status_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type, EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, start, pages, memory_type, overlap_only_ram ? "yes" : "no"); + if (start & EFI_PAGE_MASK) + panic("%s: start not aligned\n", __func__); + if (memory_type >= EFI_MAX_MEMORY_TYPE) return EFI_INVALID_PARAMETER;
The first argument has to be aligned with EFI_PAGE_SIZE. This alignment is already checked for external callers but it is not checked for internal callers. Unfortunately, most of the time the return value is not checked, so scream loud and clear. Signed-off-by: Michael Walle <michael at walle.cc> --- lib/efi_loader/efi_memory.c | 3 +++ 1 file changed, 3 insertions(+)