Additionally, fix the terminfo spelling mistake in
the GRUB development documentation.
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
This is an additional fix which has been missing from the commit 837fe48de
(i18n: Format large integers before the translation message).
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
The GNU gettext only supports the ISO C99 macros for integral
types. If there is a need to use unsupported formatting macros,
e.g. PRIuGRUB_UINT64_T, according to [1] the number to a string
conversion should be separated from the code printing message
requiring the internationalization. So, the function grub_snprintf()
is used to print the numeric values to an intermediate buffer and
the internationalized message contains a string format directive.
[1] https://www.gnu.org/software/gettext/manual/html_node/Preparing-Strings.html#No-string-concatenation
Signed-off-by: Miguel Ángel Arruga Vivas <rosen644835@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Since commit 7ce3259f67 (video/fb/fbfill: Fix potential integer
overflow), clang builds of grub-emu have failed with messages like:
/usr/bin/ld: libgrubmods.a(libgrubmods_a-fbfill.o): in function `grub_video_fbfill_direct24':
fbfill.c:(.text+0x28e): undefined reference to `__muloti4'
This appears to be due to a weird quirk in how clang compiles
grub_mul(dst->mode_info->bytes_per_pixel, width, &rowskip)
which is grub_mul(unsigned int, int, &grub_size_t).
It looks like clang somewhere promotes everything to 128-bit maths
before ultimately reducing down to 64 bit for grub_size_t. I think
this is because width is signed, and indeed converting width to an
unsigned int makes the problem go away.
This conversion also makes more sense generally:
- the caller of all the fbfill_directN functions is
grub_video_fb_fill_dispatch() and it takes width and height as
unsigned ints already,
- it doesn't make sense to fill a negative width or height.
Convert the width and height arguments and associated loop counters
to unsigned ints.
Fixes: 7ce3259f67 (video/fb/fbfill: Fix potential integer overflow)
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The ext2 (and ext3, ext4) filesystems write the number of free inodes to
location 0x410.
On a MINIX filesystem, that same location is used for the MINIX superblock
magic number.
If the number of free inodes on an ext2 filesystem is equal to any
of the four MINIX superblock magic values plus any multiple of 65536,
GRUB's MINIX filesystem code will probe it as a MINIX filesystem.
In the case of an OS using ext2 as the root filesystem, since there will
ordinarily be some amount of file creation and deletion on every bootup,
it effectively means that this situation has a 1:16384 chance of being hit
on every reboot.
This will cause GRUB's filesystem probing code to mistakenly identify an
ext2 filesystem as MINIX. This can be seen by e.g. "search --label"
incorrectly indicating that no such ext2 partition with matching label
exists, whereas in fact it does.
After spotting the rough cause of the issue I was facing here, I borrowed
much of the diagnosis/explanation from meierfra who found and investigated
the same issue in util-linux in 2010:
https://bugs.launchpad.net/ubuntu/+source/util-linux/+bug/518582
This was fixed in util-linux by having the MINIX code check for the
ext2 magic. Do the same here.
Signed-off-by: Daniel Drake <drake@endlessm.com>
Reviewed-by: Derek Foreman <derek@endlessos.org>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This is a temporary, less-intrusive change to get the build to success with
compiler format string checking turned on. There is a better fix which
addresses this issue, but it needs more testing. Use this change so that
format string checking on grub_error() can be turned on until the better
change is fully tested.
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The macro ELF_R_TYPE does not change the underlying type. Here its argument
is a 64-bit Elf64_Xword. Make sure the format code matches.
For the RISC-V architecture, rel->r_info could be either Elf32_Xword or
Elf64_Xword depending on if 32 or 64-bit RISC-V is being built. So cast
to 64-bit value regardless.
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.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 second format string argument, GRUB_EFI_MAX_USABLE_ADDRESS, is a macro
to a number literal. However, depending on what the target architecture, the
type can be 32 or 64 bits. Cast to a 64-bit integer. Also, change the
format string literals "%llx" to use PRIxGRUB_UINT64_T.
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The format code is for a 32-bit int, but the argument, keyid, is declared as
a 64 bit int. The comment above says keyid is 32-bit. I'm not sure if the
comment or declaration is wrong, so force the display of a 64-bit int for now.
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The grub_error() has a format string expecting two arguments, but only one
provided. According to the comments in the struct grub_nv_super definition,
the version field looks like a version number where major.minor is encoded
as each a byte in the two-byte short.
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Its obvious from the error message that the variable named "type" was
accidentally omitted.
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
While attempting to dual boot Microsoft Windows with UEFI chainloader,
it failed with below error when UEFI Secure Boot was enabled:
error ../../grub-core/kern/verifiers.c:119:verification requested but
nobody cares: /EFI/Microsoft/Boot/bootmgfw.efi.
It is a regression, as previously it worked without any problem.
It turns out chainloading PE image has been locked down by commit
578c95298 (kern: Add lockdown support). However, we should consider it
as verifiable object by shim to allow booting in UEFI Secure Boot mode.
The chainloaded PE image could also have trusted signature created by
vendor with their pubkey cert in db. For that matters it's usage should
not be locked down under UEFI Secure Boot, and instead shim should be
allowed to validate a PE binary signature before running it.
Fixes: 578c95298 (kern: Add lockdown support)
Signed-off-by: Michael Chang <mchang@suse.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This error message comes from the grub_print_error() in
grub_pata_device_initialize(), which does not pass on the error, and is
raised in check_device(). The function check_device() needs to return this
as an error because check_device() is also used in grub_pata_open(), which
does pass on this error to indicate that the device can not be used.
This is actually not an error when displayed by grub_pata_device_initialize()
because it just indicates that there are no pata devices seen. This may be
confusing to end users who do not have pata devices yet are loading the
pata module (perhaps implicitly via nativedisk). This also causes unnecessary
output which may need to be accounted for in functional testing.
Instead print to the debug log when check_device() raises this "error" and
pop the error from the error stack. If there is another error on the stack
then print the error stack as those should be real errors.
Signed-off-by: Glenn Washburn <development@efficientek.com>
Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
We encountered a file not found error when the symlink filesize is
equal to 60:
$ ls -l initrd
lrwxrwxrwx 1 root root 60 Jan 6 16:37 initrd -> secure-core-image-initramfs-5.10.2-yoctodev-standard.cpio.gz
When booting, we got the following error in the GRUB:
error: file `/initrd' not found
The root cause is that the size of diro->inode.symlink is equal to 60
and a symlink name has to be terminated with NUL there. So, if the
symlink filesize is exactly 60 then it is also stored in a separate
block rather than in the inode itself.
Signed-off-by: Yi Zhao <yi.zhao@windriver.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>
According to the Embedded Base Boot Requirements (EBBR) specification the
device-tree passed to Linux as a configuration table must reside in
EfiACPIReclaimMemory.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
UEFI specification 2.8 errata B introduced the EFI_RT_PROPERTIES_TABLE
describing the services available at runtime.
The lsefisystab command is used to display installed EFI configuration
tables. Currently it only shows the GUID but not a short text for the
new table.
Provide a short text for the EFI_RT_PROPERTIES_TABLE_GUID.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The commit f1957dc8a (RISC-V: Add to build system) added two entries to
the options array, but only 1 entry to the enum. This resulted in
everything after the insertion point being off by one.
This broke at least the "file --is-hibernated-hiberfil" command.
Bring the two back in sync by splitting the IS_RISCV_EFI enum entry into
two, as is done for other architectures.
Signed-off-by: Derek Foreman <derek@endlessos.org>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Fix compilation error due to missing parameter to
grub_printf() when MM_DEBUG is defined.
Fixes: 64e26162e (calloc: Make sure we always have an overflow-checking calloc() available)
Signed-off-by: Marco A Benatto <mbenatto@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The gui_progress_bar and gui_label components can display the timeout
value. The format string can be set through a theme file. This patch
adds a validation step to the format string.
If a user loads a theme file into the GRUB without this patch then
a GUI label with the following settings
+ label {
...
id = "__timeout__"
text = "%s"
}
will interpret the current timeout value as string pointer and print the
memory at that position on the screen. It is not desired behavior.
Signed-off-by: Thomas Frauendorfer | Miray Software <tf@miray.de>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The grub_printf_fmt_check() function parses the arguments of an untrusted
printf() format and an expected printf() format and then compares the
arguments counts and arguments types. The arguments count in the untrusted
format string must be less or equal to the arguments count in the expected
format string and both arguments types must match.
To do this the parse_printf_arg_fmt() helper function is extended in the
following way:
1. Add a return value to report errors to the grub_printf_fmt_check().
2. Add the fmt_check argument to enable stricter format verification:
- the function expects that arguments definitions are always
terminated by a supported conversion specifier.
- positional parameters, "$", are not allowed, as they cannot be
validated correctly with the current implementation. For example
"%s%1$d" would assign the first args entry twice while leaving the
second one unchanged.
- Return an error if preallocated space in args is too small and
allocation fails for the needed size. The grub_printf_fmt_check()
should verify all arguments. So, if validation is not possible for
any reason it should return an error.
This also adds a case entry to handle "%%", which is the escape
sequence to print "%" character.
3. Add the max_args argument to check for the maximum allowed arguments
count in a printf() string. This should be set to the arguments count
of the expected format. Then the parse_printf_arg_fmt() function will
return an error if the arguments count is exceeded.
The two additional arguments allow us to use parse_printf_arg_fmt() in
printf() and grub_printf_fmt_check() calls.
When parse_printf_arg_fmt() is used by grub_printf_fmt_check() the
function parse user provided untrusted format string too. So, in
that case it is better to be too strict than too lenient.
Signed-off-by: Thomas Frauendorfer | Miray Software <tf@miray.de>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Set printf() argument type for "%s" to new type STRING. This is in
preparation for a follow up patch to compare a printf() format string
against an expected printf() format string.
For "%s" the corresponding printf() argument is dereferenced as pointer
while all other argument types are defined as integer value. However,
when validating a printf() format it is necessary to differentiate "%s"
from "%p" and other integers. So, let's do that.
Signed-off-by: Thomas Frauendorfer | Miray Software <tf@miray.de>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This patch is preparing for a follow up patch which will use
the format parsing part to compare the arguments in a printf()
format from an external source against a printf() format with
expected arguments.
Signed-off-by: Thomas Frauendorfer | Miray Software <tf@miray.de>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Commit 32ddc42c (efi: Only register shim_lock verifier if shim_lock
protocol is found and SB enabled) reintroduced CVE-2020-15705 which
previously only existed in the out-of-tree linuxefi patches and was
fixed as part of the BootHole patch series.
Under Secure Boot enforce loading shim_lock verifier. Allow skipping
shim_lock verifier if SecureBoot/MokSBState EFI variables indicate
skipping validations, or if GRUB image is built with --disable-shim-lock.
Fixes: 132ddc42c (efi: Only register shim_lock verifier if shim_lock
protocol is found and SB enabled)
Fixes: CVE-2020-15705
Fixes: CVE-2021-3418
Reported-by: Dimitri John Ledkov <xnox@ubuntu.com>
Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
grub_parser_split_cmdline() expands variable names present in the supplied
command line in to their corresponding variable contents and uses a 1 kiB
stack buffer for temporary storage without sufficient bounds checking. If
the function is called with a command line that references a variable with
a sufficiently large payload, it is possible to overflow the stack
buffer via tab completion, corrupt the stack frame and potentially
control execution.
Fixes: CVE-2020-27749
Reported-by: Chris Coulson <chris.coulson@canonical.com>
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Add a new variable sized heap buffer type (grub_buffer_t) with simple
operations for appending data, accessing the data and maintaining
a read cursor.
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Introduce a common function epilogue used for cleaning up on all
return paths, which will simplify additional error handling to be
introduced in a subsequent commit.
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
process_char() and grub_parser_split_cmdline() use similar code for
terminating the most recent argument. Add a helper function for this.
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
grub_parser_split_cmdline() iterates over each command line character.
In order to add error checking and to simplify the subsequent error
handling, split the character processing in to a separate function.
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The getline() function supplied to grub_parser_split_cmdline() returns
a newly allocated buffer and can be called multiple times, but the
returned buffer is never freed.
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
We need to check errors before calling into a function that uses the result.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This prevents a divide by zero if nstripes == nparities, and
also prevents propagation of invalid values if nstripes ends up
less than nparities.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This prevents infinite recursion in the diskfilter verification code.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
rlocn->offset is read directly from disk and added to the metadatabuf
pointer to create a pointer to a block of metadata. It's a 64-bit
quantity so as long as you don't overflow you can set subsequent
pointers to point anywhere in memory.
Require that rlocn->offset fits within the metadata buffer size.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
We could reach the end of valid metadata and not realize, leading to
some buffer overreads. Check if we have reached the end and bail.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Clean up a bunch of cases where we could have strstr() fail and lead to
us dereferencing NULL.
We'll still leak memory in some cases (loops don't clean up allocations
from earlier iterations if a later iteration fails) but at least we're
not crashing.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
There's an if block for the presence of "physical_volumes {", but if
that block is absent, then p remains NULL and a NULL-deref will result
when looking for logical volumes.
It doesn't seem like LVM makes sense without physical volumes, so error
out rather than crashing.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This catches at least some OOB reads, and it's possible I suppose that
if 2 * mda_size is less than GRUB_LVM_MDA_HEADER_SIZE it might catch some
OOB writes too (although that hasn't showed up as a crash in fuzzing yet).
It's a bit ugly and I'd appreciate better suggestions.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
We unconditionally trusted offset_xl from the LVM label header, even if
it told us that the PV header/disk locations were way off past the end
of the data we read from disk.
Require that the offset be sane, fixing an OOB read and crash.
Fixes: CID 314367, CID 314371
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel Kiper