In grub_cmd_initrd(), memory is allocated for variable initrd_ctx
before calling grub_relocator_alloc_chunk_align_safe(). When the
function call fails, initrd_ctx should be freed before exiting
grub_cmd_initrd().
Fixes: CID 473852
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Update linux_kernel_params to match the v6.13.7 upstream version of boot_params.
Refactor most things out into structs, as the Linux kernel does.
edid_info should be a struct with "unsigned char dummy[128]" and efi_info should
be a struct as well, starting at 0x1c0. However, for backwards compatibility,
GRUB can have efi_systab at 0x1b8 and padding at 0x1bc (or padding at both spots).
This cuts into the end of edid_info. Make edid_info inline and only make it go
up to 0x1b8.
Signed-off-by: Patrick Colp <patrick.colp@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In grub_cmd_initrd(), initrd_ctx is allocated before calling
grub_relocator_alloc_chunk_align(). When that function fails,
initrd_ctx should be freed before exiting grub_cmd_initrd().
Fixes: CID 473852
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Simply returning from grub_cmd_linux() doesn't free "file" resource nor
calls grub_dl_ref(my_mod). Jump to "fail" label for proper cleanup like
other error checks do.
Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The operation kern_end - kern_start may underflow when we input it into
grub_relocator_alloc_chunk_addr() call. To avoid this we can use safe
math for this subtraction.
Fixes: CID 73845
Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The Coverity complains that we might overflow into a negative value when
setting linux_params.kernel_alignment to (1 << align). We can remedy
this by casting it to grub_uint32_t.
Fixes: CID 473876
Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This fixes naming inconsistency that goes against coding style as well
as helps to avoid potential conflicts and confusion as this constant is
used in multiple places.
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
Reviewed-by: Ross Philipson <ross.philipson@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Current code in some codepaths neither discards nor reports errors.
Properly surface the error.
While on it split 2 cases of unrelated variables both named err.
Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In grub-core/loader/i386/multiboot_mbi.c, Coverity spotted redundant code where
the variable err was being set to GRUB_ERR_NONE and then being overwritten
later without being used. Since this is unnecessary, we can remove the code
that sets err to GRUB_ERR_NONE.
Fixes: CID 428877
Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Current code implicitly assumes that aligning chunk_size + *kern_end is
the same as aligning on curload which is not the case because
chunk_size starts at zero even if *kern_end is unaligned and ALIGN_PAGE
moved curload to an aligned position but not *kern_end + chunk_size.
This fixes booting of FreeBSD with zfs module.
Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
On ia64 alignment requirements are strict. When we pass a pointer to
UUID it needs to be at least 4-byte aligned or EFI will crash.
On the other hand in device path there is no padding for UUID, so we
need 2 types in one formor another. Make 4-byte aligned and unaligned types
The code is structured in a way to accept unaligned inputs
in most cases and supply 4-byte aligned outputs.
Efiemu case is a bit ugly because there inputs and outputs are
reversed and so we need careful casts to account for this
inversion.
Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The x86_64 Linux kernel can be booted in 32-bit mode, in which case the
startup code creates a set of preliminary page tables that map the first
4 GiB of physical memory 1:1 and enables paging. This is a prerequisite
for 64-bit execution and can therefore only be implemented in 32-bit code.
The x86_64 Linux kernel can also be booted in 64-bit mode directly: this
implies that paging is already enabled and it is the responsibility of
the bootloader to ensure that the active page tables cover the entire
loaded image, including its BSS space, the size of which is described in
the image's setup header.
Given that the EFI spec mandates execution in long mode for x86_64 and
stipulates that all system memory is mapped 1:1, the Linux/x86
requirements for 64-bit entry can be met trivially when booting on
x86_64 via EFI. So, enter via the 64-bit entry point in this case.
This involves inspecting the xloadflags field in the setup header to
check whether the 64-bit entry point is supported. This field was
introduced in Linux version v3.8 (early 2013).
This change ensures that all EFI firmware tables and other assets passed
by the firmware or bootloader in memory remain mapped and accessible
throughout the early startup code.
Avoiding the drop out of long mode will also be needed to support
upcoming CPU designs that no longer implement 32-bit mode at all
(as recently announced by Intel [0]).
[0] https://www.intel.com/content/www/us/en/developer/articles/technical/envisioning-future-simplified-architecture.html
Cc: Daniel Kiper <daniel.kiper@oracle.com>
Cc: Julian Andres Klode <julian.klode@canonical.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Use the new printf format specifier %pG.
Fixes the text representation of GUIDs in the output of the lsefisystab
command (missing 4th dash).
Signed-off-by: Oliver Steffen <osteffen@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
There are 3 implementations of a GUID in GRUB. Replace them with
a common one, placed in types.h.
It uses the "packed" flavor of the GUID structs, the alignment attribute
is dropped, since it is not required.
Signed-off-by: Oliver Steffen <osteffen@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Switch the x86 based EFI platform builds to the generic EFI loader,
which exposes the initrd via the LoadFile2 protocol instead of the
x86-specific setup header. This will launch the Linux kernel via its EFI
stub, which performs its own initialization in the EFI boot services
context before calling ExitBootServices() and performing the bare metal
Linux boot.
Given that only Linux kernel versions v5.8 and later support this initrd
loading method, the existing x86 loader is retained as a fallback, which
will also be used for Linux kernels built without the EFI stub. In this
case, GRUB calls ExitBootServices() before entering the Linux kernel,
and all EFI related information is provided to the kernel via struct
boot_params in the setup header, as before.
Note that this means that booting EFI stub kernels older than v5.8 is
not supported even when not using an initrd at all. Also, the EFI
handover protocol, which has no basis in the UEFI specification, is not
implemented.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The "addr" is used to request the memory with specific ranges but the real
loadable address come from the relocator. Thus, print the final retrieved
addresses, virtual and physical, for initrd.
On the occasion migrate to PRIxGRUB_ADDR and PRIxGRUB_SIZE format specifiers.
Signed-off-by: Jeremy Szu <jeremy.szu@canonical.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The current i386 initrd is limited under 1 GiB memory and it works with
most compressed initrds (also initrd_addr_max case reported by kernel).
addr = (addr_max - aligned_size) & ~0xFFF;
Above line is used to calculate the reasonable address to store the initrd.
However, if initrd size is greater than 1 GiB or initrd_addr_max, then it
will get overflow, especially on x86_64 arch.
Therefore, add a check point to prevent it overflows as well as having
a debug log for complex story of initrd addresses.
Signed-off-by: Jeremy Szu <jeremy.szu@canonical.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Numerous register fields in the relocator state are simply not
used depending on the relocator. This causes Coverity to flag
these fields but there is no real bug here. Simply initializing
the variable to {0} solves the issue. Fixed in the else case too
for consistency.
Fixes: CID 396932
Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In the case of an error grub_initrd_load() uses argv[] to print the
filename that caused the error. It is also possible to obtain the
filename from the file handles and there is no need to duplicate that
information in argv[], so let's drop it.
Signed-off-by: Nikita Ermakov <arei@altlinux.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In grub-core/loader/i386/bsdXX.c and grub-core/loader/multiboot_elfxx.c, error
conditionals are simplified to statements such as "if (err)". Even though the
assumption that non-zero values give errors is correct, it would be clearer and
more consistent to compare these conditionals to GRUB_ERR_NONE.
Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In bsdXX.c and multiboot_elfxx.c, e_phnum is used to obtain the number of
program header table entries, but it wasn't being checked if the value was
there.
According to the elf(5) manual page,
"If the number of entries in the program header table is larger than or equal to
PN_XNUM (0xffff), this member holds PN_XNUM (0xffff) and the real number of
entries in the program header table is held in the sh_info member of the
initial entry in section header table. Otherwise, the sh_info member of the
initial entry contains the value zero."
Since this check wasn't being made, grub_elfXX_get_phnum() is being added to
elfXX.c to make this check and use e_phnum if it doesn't have PN_XNUM as a
value, else use sh_info. We also need to make sure e_phnum isn't greater than
PN_XNUM and sh_info isn't less than PN_XNUM.
Note that even though elf.c and elfXX.c are located in grub-core/kern, they are
compiled as modules and don't need the EXPORT_FUNC() macro to define the functions
in elf.h.
Also, changed casts of phnum to match variables being set as well as dropped
casts when unnecessary.
Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In bsdXX.c and multiboot_elfxx.c, e_shnum is used to obtain the number of
section header table entries, but it wasn't being checked if the value was
there.
According to the elf(5) manual page,
"If the number of entries in the section header table is larger than or equal to
SHN_LORESERVE (0xff00), e_shnum holds the value zero and the real number of
entries in the section header table is held in the sh_size member of the initial
entry in section header table. Otherwise, the sh_size member of the initial
entry in the section header table holds the value zero."
Since this check wasn't being made, grub_elfXX_get_shnum() is being added to
elfXX.c to make this check and use whichever member doesn't have a value of
zero. If both are zero, then we must return an error. We also need to make sure
that e_shnum doesn't have a value greater than or equal to SHN_LORESERVE and
sh_size isn't less than SHN_LORESERVE.
In order to get this function to work, the type ElfXX_Shnum is being added where
Elf32_Shnum defines Elf32_Word and Elf64_Shnum defines Elf64_Xword. This new
type is needed because if shnum obtains a value from sh_size, sh_size could be
of type El32_Word for Elf32_Shdr structures or Elf64_Xword for Elf64_Shdr
structures.
Note that even though elf.c and elfXX.c are located in grub-core/kern, they are
compiled as modules and don't need the EXPORT_FUNC() macro to define the functions
in elf.h.
For a few smaller changes, changed casts of shnum to match variables being set
as well as dropped casts when unnecessary and fixed spacing errors in bsdXX.c.
Also, shnum is an unsigned integer and is compared to int i in multiboot_elfxx.c,
it should be unsigned to match shnum.
Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In bsdXX.c, a couple of untrusted loop bound and untrusted allocation size bugs
were flagged by Coverity in the functions grub_openbsd_find_ramdisk() and
grub_freebsd_load_elfmodule(). These bugs were flagged by coverity because the
variable shdr was downcasting from a char pointer to an Elf_Shdr pointer
whenever it was used to set the base value in for loops. To avoid this, we need
to set shdr as an Elf_Shdr pointer where it is initialized.
In the function read_headers(), the function is reading elf section header data
from a file and passing it to the variable shdr as data for a char pointer. If
we switch the type of shdr to an Elf_Shdr pointer in read_headers() as well as
other functions, then we won't need to downcast to an Elf_Shdr pointer. By doing
this, the issue becomes masked from Coverity's view. In the following patches,
we check limits to ensure the data isn't tainted.
Also, switched use of (char *) to (grub_uint8_t *) to give a better indication
of pointer arithmetic and not suggest use of a C string.
Fixes: CID 314018
Fixes: CID 314030
Fixes: CID 314031
Fixes: CID 314039
Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
The GRUB is failing to build with GCC-12 in many places like this:
In function 'init_cbfsdisk',
inlined from 'grub_mod_init' at ../../grub-core/fs/cbfs.c:391:3:
../../grub-core/fs/cbfs.c:345:7: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
345 | ptr = *(grub_uint32_t *) 0xfffffffc;
| ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This is caused by GCC regression in 11/12 [1]. In a nut shell, the
warning is about detected invalid accesses at non-zero offsets to NULL
pointers. Since hardwired constant address is treated as NULL plus an
offset in the same underlying code, the warning is therefore triggered.
Instead of inserting #pragma all over the places where literal pointers
are accessed to avoid diagnosing array-bounds, we can try to borrow the
idea from Linux kernel that the absolute_pointer() macro [2][3] is used
to disconnect a pointer using literal address from it's original object,
hence GCC won't be able to make assumptions on the boundary while doing
pointer arithmetic. With that we can greatly reduce the code we have to
cover up by making initial literal pointer assignment to use the new
wrapper but not having to track everywhere literal pointers are
accessed. This also makes code looks cleaner.
Please note the grub_absolute_pointer() macro requires to be invoked in
a function as long as it is compound expression. Some global variables
with literal pointers has been changed to local ones in order to use
grub_absolute_pointer() to initialize it. The shuffling is basically done
in a selective and careful way that the variable's scope doesn't matter
being local or global, for example, the global variable must not get
modified at run time throughout. For the record, here's the list of
global variables got shuffled in this patch:
grub-core/commands/i386/pc/drivemap.c:int13slot
grub-core/term/i386/pc/console.c:bios_data_area
grub-core/term/ns8250.c:serial_hw_io_addr
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
[2] https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler.h#L180
[3] https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler-gcc.h#L31
Signed-off-by: Michael Chang <mchang@suse.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In the function grub_xnu_boot(), struct grub_relocator32_state state is called
but isn't being initialized. This results in the members grub_uint32_t ebx,
grub_uint32_t ecx, grub_uint32_t edx, grub_uint32_t edi, and grub_uint32_t esi
being filled with junk data from the stack since none of them are being set to
any values. We can prevent this by setting state to {0}.
Fixes: CID 375035
Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In the function grub_xnu_boot_resume(), struct grub_relocator32_state state is
called but isn't being initialized. This results in the members grub_uint32_t
ebx, grub_uint32_t ecx, grub_uint32_t edx, grub_uint32_t esi, and grub_uint32_t
edi being filled with junk data from the stack since none of them are being set
to any values. We can prevent this by setting state to {0}.
Fixes: CID 375031
Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In the function grub_linux16_boot(), struct grub_relocator16_state state is
called but isn't being initialized. This results in the members grub_uint32_t
ebx, grub_uint32_t edx, grub_uint32_t esi, and grub_uint32_t ebp being filled
with junk data from the stack since none of them are being set to any values.
We can prevent this by setting state to {0}.
Fixes: CID 375028
Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In the function grub_netbsd_setup_video(), struct grub_netbsd_btinfo_framebuf
params is called but isn't being initialized. The member grub_uint8_t
reserved[16] isn't set to any values and is instead filled with junk data from
the stack. We can prevent this by setting params to {0}.
Fixes: CID 375026
Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Latest GCC may complain in that way:
In file included from ../include/grub/disk.h:31,
from ../include/grub/file.h:26,
from ../include/grub/loader.h:23,
from loader/i386/bsd.c:19:
loader/i386/bsd.c: In function ‘grub_cmd_openbsd’:
../include/grub/misc.h:71:10: error: ‘ptr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
71 | return grub_memmove (dest, src, n);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
loader/i386/bsd.c:266:9: note: ‘ptr’ was declared here
266 | void *ptr;
| ^~~
So, let's fix it by assigning NULL to ptr in grub_bsd_add_meta().
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Robbie Harwood <rharwood@redhat.com>
Also remove casting of format string args so that the architecture dependent
type is preserved.
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The relocatable variable is defined as grub_uint8_t. Relevant
member in setup_header structure is also defined as one byte
in Linux boot protocol. By semantic definition it is a bool type.
It is not appropriate to treat it as a four bytes. This patch
fixes the issue.
Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The preferred_address has been assigned to GRUB_LINUX_BZIMAGE_ADDR
during initialization in grub_cmd_linux(). The assignment here
is redundant and should be removed.
Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The code in the next block suggests that it is possible for .set to be
true but .arg may still be NULL.
This code assumes that it is never NULL, yet later is testing if it is
NULL - that is inconsistent.
So we should check first if .arg is not NULL, and remove this check that
is being flagged by Coverity since it is no longer required.
Fixes: CID 292471
Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Now that the GRUB has a grub_efi_get_secureboot() function to check the
UEFI Secure Boot status, use it to report that to the Linux kernel.
Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Signed-off-by: Marco A Benatto <mbenatto@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Several places we take the length of a device path and subtract 4 from
it, without ever checking that it's >= 4. There are also cases where
this kind of malformation will result in unpredictable iteration,
including treating the length from one dp node as the type in the next
node. These are all errors, no matter where the data comes from.
This patch adds a checking macro, GRUB_EFI_DEVICE_PATH_VALID(), which
can be used in several places, and makes GRUB_EFI_NEXT_DEVICE_PATH()
return NULL and GRUB_EFI_END_ENTIRE_DEVICE_PATH() evaluate as true when
the length is too small. Additionally, it makes several places in the
code check for and return errors in these cases.
Signed-off-by: Peter Jones <pjones@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This commit introduces integer underflow mitigation in max_addr calculation
in grub_relocator_alloc_chunk_align() invocation.
It consists of 2 fixes:
1. Introduced grub_relocator_alloc_chunk_align_safe() wrapper function to perform
sanity check for min/max and size values, and to make safe invocation of
grub_relocator_alloc_chunk_align() with validated max_addr value. Replace all
invocations such as grub_relocator_alloc_chunk_align(..., min_addr, max_addr - size, size, ...)
by grub_relocator_alloc_chunk_align_safe(..., min_addr, max_addr, size, ...).
2. Introduced UP_TO_TOP32(s) macro for the cases where max_addr is 32-bit top
address (0xffffffff - size + 1) or similar.
Signed-off-by: Alexey Makhalov <amakhalov@vmware.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Use arithmetic macros from safemath.h to accomplish it. In this commit,
I didn't want to be too paranoid to check every possible math equation
for overflow/underflow. Only obvious places (with non zero chance of
overflow/underflow) were refactored.
Signed-off-by: Alexey Makhalov <amakhalov@vmware.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
grub_xnu_devprop_add_property() should not free utf8 and utf16 as it get
allocated and freed in the caller.
Minor improvement: do prop fields initialization after memory allocations.
Fixes: CID 292442, CID 292457, CID 292460, CID 292466
Signed-off-by: Alexey Makhalov <amakhalov@vmware.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This attempts to fix the places where we do the following where
arithmetic_expr may include unvalidated data:
X = grub_malloc(arithmetic_expr);
It accomplishes this by doing the arithmetic ahead of time using grub_add(),
grub_sub(), grub_mul() and testing for overflow before proceeding.
Among other issues, this fixes:
- allocation of integer overflow in grub_video_bitmap_create()
reported by Chris Coulson,
- allocation of integer overflow in grub_png_decode_image_header()
reported by Chris Coulson,
- allocation of integer overflow in grub_squash_read_symlink()
reported by Chris Coulson,
- allocation of integer overflow in grub_ext2_read_symlink()
reported by Chris Coulson,
- allocation of integer overflow in read_section_as_string()
reported by Chris Coulson.
Fixes: CVE-2020-14309, CVE-2020-14310, CVE-2020-14311
Signed-off-by: Peter Jones <pjones@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This modifies most of the places we do some form of:
X = malloc(Y * Z);
to use calloc(Y, Z) instead.
Among other issues, this fixes:
- allocation of integer overflow in grub_png_decode_image_header()
reported by Chris Coulson,
- allocation of integer overflow in luks_recover_key()
reported by Chris Coulson,
- allocation of integer overflow in grub_lvm_detect()
reported by Chris Coulson.
Fixes: CVE-2020-14308
Signed-off-by: Peter Jones <pjones@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Currently the string functions grub_strtol(), grub_strtoul(), and
grub_strtoull() don't declare the "end" pointer in such a way as to
require the pointer itself or the character array to be immutable to the
implementation, nor does the C standard do so in its similar functions,
though it does require us not to change any of it.
The typical declarations of these functions follow this pattern:
long
strtol(const char * restrict nptr, char ** restrict endptr, int base);
Much of the reason for this is historic, and a discussion of that
follows below, after the explanation of this change. (GRUB currently
does not include the "restrict" qualifiers, and we name the arguments a
bit differently.)
The implementation is semantically required to treat the character array
as immutable, but such accidental modifications aren't stopped by the
compiler, and the semantics for both the callers and the implementation
of these functions are sometimes also helped by adding that requirement.
This patch changes these declarations to follow this pattern instead:
long
strtol(const char * restrict nptr,
const char ** const restrict endptr,
int base);
This means that if any modification to these functions accidentally
introduces either an errant modification to the underlying character
array, or an accidental assignment to endptr rather than *endptr, the
compiler should generate an error. (The two uses of "restrict" in this
case basically mean strtol() isn't allowed to modify the character array
by going through *endptr, and endptr isn't allowed to point inside the
array.)
It also means the typical use case changes to:
char *s = ...;
const char *end;
long l;
l = strtol(s, &end, 10);
Or even:
const char *p = str;
while (p && *p) {
long l = strtol(p, &p, 10);
...
}
This fixes 26 places where we discard our attempts at treating the data
safely by doing:
const char *p = str;
long l;
l = strtol(p, (char **)&ptr, 10);
It also adds 5 places where we do:
char *p = str;
while (p && *p) {
long l = strtol(p, (const char ** const)&p, 10);
...
/* more calls that need p not to be pointer-to-const */
}
While moderately distasteful, this is a better problem to have.
With one minor exception, I have tested that all of this compiles
without relevant warnings or errors, and that /much/ of it behaves
correctly, with gcc 9 using 'gcc -W -Wall -Wextra'. The one exception
is the changes in grub-core/osdep/aros/hostdisk.c , which I have no idea
how to build.
Because the C standard defined type-qualifiers in a way that can be
confusing, in the past there's been a slow but fairly regular stream of
churn within our patches, which add and remove the const qualifier in many
of the users of these functions. This change should help avoid that in
the future, and in order to help ensure this, I've added an explanation
in misc.h so that when someone does get a compiler warning about a type
error, they have the fix at hand.
The reason we don't have "const" in these calls in the standard is
purely anachronistic: C78 (de facto) did not have type qualifiers in the
syntax, and the "const" type qualifier was added for C89 (I think; it
may have been later). strtol() appears to date from 4.3BSD in 1986,
which means it could not be added to those functions in the standard
without breaking compatibility, which is usually avoided.
The syntax chosen for type qualifiers is what has led to the churn
regarding usage of const, and is especially confusing on string
functions due to the lack of a string type. Quoting from C99, the
syntax is:
declarator:
pointer[opt] direct-declarator
direct-declarator:
identifier
( declarator )
direct-declarator [ type-qualifier-list[opt] assignment-expression[opt] ]
...
direct-declarator [ type-qualifier-list[opt] * ]
...
pointer:
* type-qualifier-list[opt]
* type-qualifier-list[opt] pointer
type-qualifier-list:
type-qualifier
type-qualifier-list type-qualifier
...
type-qualifier:
const
restrict
volatile
So the examples go like:
const char foo; // immutable object
const char *foo; // mutable pointer to object
char * const foo; // immutable pointer to mutable object
const char * const foo; // immutable pointer to immutable object
const char const * const foo; // XXX extra const keyword in the middle
const char * const * const foo; // immutable pointer to immutable
// pointer to immutable object
const char ** const foo; // immutable pointer to mutable pointer
// to immutable object
Making const left-associative for * and right-associative for everything
else may not have been the best choice ever, but here we are, and the
inevitable result is people using trying to use const (as they should!),
putting it at the wrong place, fighting with the compiler for a bit, and
then either removing it or typecasting something in a bad way. I won't
go into describing restrict, but its syntax has exactly the same issue
as with const.
Anyway, the last example above actually represents the *behavior* that's
required of strtol()-like functions, so that's our choice for the "end"
pointer.
Signed-off-by: Peter Jones <pjones@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Recent work around x86 Linux kernel loader revealed an underflow in the
setup_header length calculation and another related issue. Both lead to
the memory overwrite and later machine crash.
Currently when the GRUB copies the setup_header into the linux_params
(struct boot_params, traditionally known as "zero page") it assumes the
setup_header size as sizeof(linux_i386_kernel_header/lh). This is
incorrect. It should use the value calculated accordingly to the Linux
kernel boot protocol. Otherwise in case of pretty old kernel, to be
exact Linux kernel boot protocol, the GRUB may write more into
linux_params than it was expected to. Fortunately this is not very big
issue. Though it has to be fixed. However, there is also an underflow
which is grave. It happens when
sizeof(linux_i386_kernel_header/lh) > "real size of the setup_header".
Then len value wraps around and grub_file_read() reads whole kernel into
the linux_params overwriting memory past it. This leads to the GRUB
memory allocator breakage and finally to its crash during boot.
The patch fixes both issues. Additionally, it moves the code not related to
grub_memset(linux_params)/grub_memcpy(linux_params)/grub_file_read(linux_params)
section outside of it to not confuse the reader.
Fixes: e683cfb0cf (loader/i386/linux: Calculate the setup_header length)
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Ross Philipson <ross.philipson@oracle.com>
Reviewed-by: Krystian Hebel <krystian.hebel@3mdeb.com>
Previously the setup_header length was just assumed to be the size of the
linux_kernel_params struct. The linux x86 32-bit boot protocol says that the
end of the linux_i386_kernel_header is at 0x202 + the byte value at 0x201 in
the linux_i386_kernel_header. So, calculate the size of the header using the
end of the linux_i386_kernel_header, rather than assume it is the size of the
linux_kernel_params struct.
Additionally, add some required members to the linux_kernel_params
struct and align the content of linux_i386_kernel_header struct with
it. New members naming was taken directly from Linux kernel source.
linux_kernel_params and linux_i386_kernel_header structs require more
cleanup. However, this is not urgent, so, let's do this after release.
Just in case...
Signed-off-by: Andrew Jeddeloh <andrew.jeddeloh@coreos.com>
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Vladimir Serbinenko <phcoder@google.com>
Reviewed-by: Ross Philipson <ross.philipson@oracle.com>
Mirror behaviour of ELF loader in libxc: first look for Xen notes in
PT_NOTE segment, then in SHT_NOTE section and only then fallback to
a section with __xen_guest name. This fixes loading PV kernels that
Xen note have outside of PT_NOTE. While this may be result of a buggy
linker script, loading such kernel directly works fine, so make it work
with GRUB too. Specifically, this applies to binaries built from Unikraft.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Add the hooks to current code needed for Xen PVH. They will be filled
with code later when the related functionality is being added.
loader/i386/linux.c needs to include machine/kernel.h now as it needs
to get GRUB_KERNEL_USE_RSDP_ADDR from there. This in turn requires to
add an empty kernel.h header for some i386 platforms (efi, coreboot,
ieee1275, xen) and for x86_64 efi.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Hans van Kranenburg <hans@knorrie.org>
Lidong Chen