| Message ID | 20241001102239.2609631-3-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/boot: Reduce assembly code | expand |
On 01.10.2024 12:22, Frediano Ziglio wrote: > --- a/xen/arch/x86/efi/mbi2.c > +++ b/xen/arch/x86/efi/mbi2.c > @@ -13,6 +13,7 @@ efi_multiboot2_prelude(uint32_t magic, const multiboot2_fixed_t *mbi) > EFI_HANDLE ImageHandle = NULL; > EFI_SYSTEM_TABLE *SystemTable = NULL; > const char *cmdline = NULL; > + const void *const mbi_raw = (const void *)mbi; > bool have_bs = false; > > if ( magic != MULTIBOOT2_BOOTLOADER_MAGIC ) > @@ -21,7 +22,9 @@ efi_multiboot2_prelude(uint32_t magic, const multiboot2_fixed_t *mbi) > /* Skip Multiboot2 information fixed part. */ > tag = _p(ROUNDUP((unsigned long)(mbi + 1), MULTIBOOT2_TAG_ALIGN)); > > - for ( ; (const void *)tag - (const void *)mbi < mbi->total_size && > + for ( ; (const void *)(tag + 1) - mbi_raw <= mbi->total_size && > + tag->size >= sizeof(*tag) && > + (const void *)tag + tag->size - mbi_raw <= mbi->total_size && > tag->type != MULTIBOOT2_TAG_TYPE_END; > tag = _p(ROUNDUP((unsigned long)tag + tag->size, > MULTIBOOT2_TAG_ALIGN)) ) Hmm, looks like what I said on the earlier version still wasn't unambiguous enough; I'm sorry. There is still potential for intermediate overflows in the calculations. _If_ we care about avoiding overflows, we need to avoid all of that. Even more so that Misra may not like this sort of pointer calculations. You know tag >= mbi_raw, so tag - mbi_raw is always valid to calculate. tag->size can further be checked to be less than mbi->total_size, at which point mbi->total_size - tag->size can also be calculated without risking {over,under}flow. (Similar then for the earlier (tag + 1) check.) Jan
On Tue, Oct 1, 2024 at 5:02 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 01.10.2024 12:22, Frediano Ziglio wrote: > > --- a/xen/arch/x86/efi/mbi2.c > > +++ b/xen/arch/x86/efi/mbi2.c > > @@ -13,6 +13,7 @@ efi_multiboot2_prelude(uint32_t magic, const multiboot2_fixed_t *mbi) > > EFI_HANDLE ImageHandle = NULL; > > EFI_SYSTEM_TABLE *SystemTable = NULL; > > const char *cmdline = NULL; > > + const void *const mbi_raw = (const void *)mbi; > > bool have_bs = false; > > > > if ( magic != MULTIBOOT2_BOOTLOADER_MAGIC ) > > @@ -21,7 +22,9 @@ efi_multiboot2_prelude(uint32_t magic, const multiboot2_fixed_t *mbi) > > /* Skip Multiboot2 information fixed part. */ > > tag = _p(ROUNDUP((unsigned long)(mbi + 1), MULTIBOOT2_TAG_ALIGN)); > > > > - for ( ; (const void *)tag - (const void *)mbi < mbi->total_size && > > + for ( ; (const void *)(tag + 1) - mbi_raw <= mbi->total_size && > > + tag->size >= sizeof(*tag) && > > + (const void *)tag + tag->size - mbi_raw <= mbi->total_size && > > tag->type != MULTIBOOT2_TAG_TYPE_END; > > tag = _p(ROUNDUP((unsigned long)tag + tag->size, > > MULTIBOOT2_TAG_ALIGN)) ) > > Hmm, looks like what I said on the earlier version still wasn't unambiguous > enough; I'm sorry. There is still potential for intermediate overflows in > the calculations. _If_ we care about avoiding overflows, we need to avoid > all of that. Even more so that Misra may not like this sort of pointer > calculations. You know tag >= mbi_raw, so tag - mbi_raw is always valid to > calculate. tag->size can further be checked to be less than mbi->total_size, > at which point mbi->total_size - tag->size can also be calculated without > risking {over,under}flow. (Similar then for the earlier (tag + 1) check.) > > Jan Hi, personally, I don't care much about checking for overflows here. It's not that we are in a higher security level, and we need to check malicious intentions (like when user calls the kernel or a VM calls the hypervisor), if the loader (which is at the same security level) is passing garbage we are going to crash. Saying that with this commit Marek checks are failing, I was thinking about dropping this commit completely. Yes, in theory better checks are better, but if we cause regression and boot failures maybe it's better to allow some wrong data. That's one reason I wanted to keep this commit separate from the other, which is just translating what assembly code was doing, not introducing any regression (good or bad) in behavior. I think it would be worth investigating what Marek machine is passing in order to make sure we accept whatever wrong data we are receiving (or understanding why more checks cause the issue). Frediano
diff --git a/xen/arch/x86/efi/mbi2.c b/xen/arch/x86/efi/mbi2.c index 55a1777483..935f3ae5d0 100644 --- a/xen/arch/x86/efi/mbi2.c +++ b/xen/arch/x86/efi/mbi2.c @@ -13,6 +13,7 @@ efi_multiboot2_prelude(uint32_t magic, const multiboot2_fixed_t *mbi) EFI_HANDLE ImageHandle = NULL; EFI_SYSTEM_TABLE *SystemTable = NULL; const char *cmdline = NULL; + const void *const mbi_raw = (const void *)mbi; bool have_bs = false; if ( magic != MULTIBOOT2_BOOTLOADER_MAGIC ) @@ -21,7 +22,9 @@ efi_multiboot2_prelude(uint32_t magic, const multiboot2_fixed_t *mbi) /* Skip Multiboot2 information fixed part. */ tag = _p(ROUNDUP((unsigned long)(mbi + 1), MULTIBOOT2_TAG_ALIGN)); - for ( ; (const void *)tag - (const void *)mbi < mbi->total_size && + for ( ; (const void *)(tag + 1) - mbi_raw <= mbi->total_size && + tag->size >= sizeof(*tag) && + (const void *)tag + tag->size - mbi_raw <= mbi->total_size && tag->type != MULTIBOOT2_TAG_TYPE_END; tag = _p(ROUNDUP((unsigned long)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) )
Tag structure should contain at least the tag header. Entire tag structure must be contained inside MBI2 data. Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> --- Changes since v6: - compare against total_size every time to avoid overflows. --- xen/arch/x86/efi/mbi2.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)