| Message ID | 20200924160546.8967-1-ardb@kernel.org |
|---|---|
| State | Accepted |
| Commit | b89114cd018cffa5deb7def1844ce1891efd4f96 |
| Headers | show |
| Series | efi: mokvar-table: fix some issues in new code | expand |
On Thu, Sep 24, 2020 at 06:05:46PM +0200, Ard Biesheuvel wrote: > Fix a couple of issues in the new mokvar-table handling code, as > pointed out by Arvind and Boris: > - don't bother checking the end of the physical region against the start > address of the mokvar table, > - ensure that we enter the loop with err = -EINVAL, > - replace size_t with unsigned long to appease pedantic type equality > checks. > > Cc: Lenny Szubowicz <lszubowi@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Arvind Sankar <nivedita@alum.mit.edu> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Reviewed-by: Arvind Sankar <nivedita@alum.mit.edu> > --- > drivers/firmware/efi/mokvar-table.c | 25 +++++++++++-------------- > 1 file changed, 11 insertions(+), 14 deletions(-) > > diff --git a/drivers/firmware/efi/mokvar-table.c b/drivers/firmware/efi/mokvar-table.c > index b1cd49893d4d..72a9e1736fef 100644 > --- a/drivers/firmware/efi/mokvar-table.c > +++ b/drivers/firmware/efi/mokvar-table.c > @@ -98,15 +98,14 @@ static struct kobject *mokvar_kobj; > void __init efi_mokvar_table_init(void) > { > efi_memory_desc_t md; > - u64 end_pa; > void *va = NULL; > - size_t cur_offset = 0; > - size_t offset_limit; > - size_t map_size = 0; > - size_t map_size_needed = 0; > - size_t size; > + unsigned long cur_offset = 0; > + unsigned long offset_limit; > + unsigned long map_size = 0; > + unsigned long map_size_needed = 0; > + unsigned long size; > struct efi_mokvar_table_entry *mokvar_entry; > - int err = -EINVAL; > + int err; > > if (!efi_enabled(EFI_MEMMAP)) > return; > @@ -122,18 +121,16 @@ void __init efi_mokvar_table_init(void) > pr_warn("EFI MOKvar config table is not within the EFI memory map\n"); > return; > } > - end_pa = efi_mem_desc_end(&md); > - if (efi.mokvar_table >= end_pa) { > - pr_err("EFI memory descriptor containing MOKvar config table is invalid\n"); > - return; > - } > - offset_limit = end_pa - efi.mokvar_table; > + > + offset_limit = efi_mem_desc_end(&md) - efi.mokvar_table; > + > /* > * Validate the MOK config table. Since there is no table header > * from which we could get the total size of the MOK config table, > * we compute the total size as we validate each variably sized > * entry, remapping as necessary. > */ > + err = -EINVAL; > while (cur_offset + sizeof(*mokvar_entry) <= offset_limit) { > mokvar_entry = va + cur_offset; > map_size_needed = cur_offset + sizeof(*mokvar_entry); > @@ -150,7 +147,7 @@ void __init efi_mokvar_table_init(void) > offset_limit); > va = early_memremap(efi.mokvar_table, map_size); > if (!va) { > - pr_err("Failed to map EFI MOKvar config table pa=0x%lx, size=%zu.\n", > + pr_err("Failed to map EFI MOKvar config table pa=0x%lx, size=%lu.\n", > efi.mokvar_table, map_size); > return; > } > -- > 2.17.1 >
On 9/24/20 3:12 PM, Arvind Sankar wrote: > On Thu, Sep 24, 2020 at 06:05:46PM +0200, Ard Biesheuvel wrote: >> Fix a couple of issues in the new mokvar-table handling code, as >> pointed out by Arvind and Boris: >> - don't bother checking the end of the physical region against the start >> address of the mokvar table, >> - ensure that we enter the loop with err = -EINVAL, >> - replace size_t with unsigned long to appease pedantic type equality >> checks. >> >> Cc: Lenny Szubowicz <lszubowi@redhat.com> >> Cc: Borislav Petkov <bp@alien8.de> >> Cc: Arvind Sankar <nivedita@alum.mit.edu> >> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > Reviewed-by: Arvind Sankar <nivedita@alum.mit.edu> Reviewed-by: Lenny Szubowicz <lszubowi@redhat.com> > >> --- >> drivers/firmware/efi/mokvar-table.c | 25 +++++++++++-------------- >> 1 file changed, 11 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/firmware/efi/mokvar-table.c b/drivers/firmware/efi/mokvar-table.c >> index b1cd49893d4d..72a9e1736fef 100644 >> --- a/drivers/firmware/efi/mokvar-table.c >> +++ b/drivers/firmware/efi/mokvar-table.c >> @@ -98,15 +98,14 @@ static struct kobject *mokvar_kobj; >> void __init efi_mokvar_table_init(void) >> { >> efi_memory_desc_t md; >> - u64 end_pa; >> void *va = NULL; >> - size_t cur_offset = 0; >> - size_t offset_limit; >> - size_t map_size = 0; >> - size_t map_size_needed = 0; >> - size_t size; >> + unsigned long cur_offset = 0; >> + unsigned long offset_limit; >> + unsigned long map_size = 0; >> + unsigned long map_size_needed = 0; >> + unsigned long size; >> struct efi_mokvar_table_entry *mokvar_entry; >> - int err = -EINVAL; >> + int err; >> >> if (!efi_enabled(EFI_MEMMAP)) >> return; >> @@ -122,18 +121,16 @@ void __init efi_mokvar_table_init(void) >> pr_warn("EFI MOKvar config table is not within the EFI memory map\n"); >> return; >> } >> - end_pa = efi_mem_desc_end(&md); >> - if (efi.mokvar_table >= end_pa) { >> - pr_err("EFI memory descriptor containing MOKvar config table is invalid\n"); >> - return; >> - } >> - offset_limit = end_pa - efi.mokvar_table; >> + >> + offset_limit = efi_mem_desc_end(&md) - efi.mokvar_table; >> + >> /* >> * Validate the MOK config table. Since there is no table header >> * from which we could get the total size of the MOK config table, >> * we compute the total size as we validate each variably sized >> * entry, remapping as necessary. >> */ >> + err = -EINVAL; >> while (cur_offset + sizeof(*mokvar_entry) <= offset_limit) { >> mokvar_entry = va + cur_offset; >> map_size_needed = cur_offset + sizeof(*mokvar_entry); >> @@ -150,7 +147,7 @@ void __init efi_mokvar_table_init(void) >> offset_limit); >> va = early_memremap(efi.mokvar_table, map_size); >> if (!va) { >> - pr_err("Failed to map EFI MOKvar config table pa=0x%lx, size=%zu.\n", >> + pr_err("Failed to map EFI MOKvar config table pa=0x%lx, size=%lu.\n", >> efi.mokvar_table, map_size); >> return; >> } >> -- >> 2.17.1 >> >
On Thu, Sep 24, 2020 at 06:05:46PM +0200, Ard Biesheuvel wrote: > Fix a couple of issues in the new mokvar-table handling code, as > pointed out by Arvind and Boris: > - don't bother checking the end of the physical region against the start > address of the mokvar table, > - ensure that we enter the loop with err = -EINVAL, > - replace size_t with unsigned long to appease pedantic type equality > checks. > > Cc: Lenny Szubowicz <lszubowi@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Arvind Sankar <nivedita@alum.mit.edu> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > drivers/firmware/efi/mokvar-table.c | 25 +++++++++++-------------- > 1 file changed, 11 insertions(+), 14 deletions(-) i386 allmodconfig looks good. Tested-by: Borislav Petkov <bp@suse.de> Thx.
On Tue, 29 Sep 2020 at 18:58, Borislav Petkov <bp@alien8.de> wrote: > > On Thu, Sep 24, 2020 at 06:05:46PM +0200, Ard Biesheuvel wrote: > > Fix a couple of issues in the new mokvar-table handling code, as > > pointed out by Arvind and Boris: > > - don't bother checking the end of the physical region against the start > > address of the mokvar table, > > - ensure that we enter the loop with err = -EINVAL, > > - replace size_t with unsigned long to appease pedantic type equality > > checks. > > > > Cc: Lenny Szubowicz <lszubowi@redhat.com> > > Cc: Borislav Petkov <bp@alien8.de> > > Cc: Arvind Sankar <nivedita@alum.mit.edu> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > drivers/firmware/efi/mokvar-table.c | 25 +++++++++++-------------- > > 1 file changed, 11 insertions(+), 14 deletions(-) > > i386 allmodconfig looks good. > > Tested-by: Borislav Petkov <bp@suse.de> > Thanks Boris. I'll have a PR out shortly.
diff --git a/drivers/firmware/efi/mokvar-table.c b/drivers/firmware/efi/mokvar-table.c index b1cd49893d4d..72a9e1736fef 100644 --- a/drivers/firmware/efi/mokvar-table.c +++ b/drivers/firmware/efi/mokvar-table.c @@ -98,15 +98,14 @@ static struct kobject *mokvar_kobj; void __init efi_mokvar_table_init(void) { efi_memory_desc_t md; - u64 end_pa; void *va = NULL; - size_t cur_offset = 0; - size_t offset_limit; - size_t map_size = 0; - size_t map_size_needed = 0; - size_t size; + unsigned long cur_offset = 0; + unsigned long offset_limit; + unsigned long map_size = 0; + unsigned long map_size_needed = 0; + unsigned long size; struct efi_mokvar_table_entry *mokvar_entry; - int err = -EINVAL; + int err; if (!efi_enabled(EFI_MEMMAP)) return; @@ -122,18 +121,16 @@ void __init efi_mokvar_table_init(void) pr_warn("EFI MOKvar config table is not within the EFI memory map\n"); return; } - end_pa = efi_mem_desc_end(&md); - if (efi.mokvar_table >= end_pa) { - pr_err("EFI memory descriptor containing MOKvar config table is invalid\n"); - return; - } - offset_limit = end_pa - efi.mokvar_table; + + offset_limit = efi_mem_desc_end(&md) - efi.mokvar_table; + /* * Validate the MOK config table. Since there is no table header * from which we could get the total size of the MOK config table, * we compute the total size as we validate each variably sized * entry, remapping as necessary. */ + err = -EINVAL; while (cur_offset + sizeof(*mokvar_entry) <= offset_limit) { mokvar_entry = va + cur_offset; map_size_needed = cur_offset + sizeof(*mokvar_entry); @@ -150,7 +147,7 @@ void __init efi_mokvar_table_init(void) offset_limit); va = early_memremap(efi.mokvar_table, map_size); if (!va) { - pr_err("Failed to map EFI MOKvar config table pa=0x%lx, size=%zu.\n", + pr_err("Failed to map EFI MOKvar config table pa=0x%lx, size=%lu.\n", efi.mokvar_table, map_size); return; }
Fix a couple of issues in the new mokvar-table handling code, as pointed out by Arvind and Boris: - don't bother checking the end of the physical region against the start address of the mokvar table, - ensure that we enter the loop with err = -EINVAL, - replace size_t with unsigned long to appease pedantic type equality checks. Cc: Lenny Szubowicz <lszubowi@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Arvind Sankar <nivedita@alum.mit.edu> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- drivers/firmware/efi/mokvar-table.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)