[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v7 2/2] x86/boot: Improve MBI2 structure check
On Tue, Oct 1, 2024 at 5:02 PM Jan Beulich <jbeulich@xxxxxxxx> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |