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>
The "ARM\x64" magic number in the file header identifies an image as one
that implements the bare metal boot protocol, allowing the loader to
simply move the file to a suitably aligned address in memory, with
sufficient headroom for the trailing .bss segment (the required memory
size is described in the header as well).
Note of this matters for GRUB, as it only supports EFI boot. EFI does
not care about this magic number, and nor should GRUB: this prevents us
from booting other PE linux images, such as the generic EFI zboot
decompressor, which is a pure PE/COFF image, and does not implement the
bare metal boot protocol.
So drop the magic number check.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Update the read hook to take into account encrypted volumes on a partition.
GRUB disk read hooks supply an absolute sector number at which the read is
started from. If the encrypted volume is in a partition, the sector number
given to the read hook will be offset by the number of the sector at the
start of the partition. The read hook then needs to subtract the partition
start from the supplied sector to get the correct start sector for the read
into the detached header file.
Reported-by: brutser <brutser@perso.be>
Signed-off-by: Glenn Washburn <development@efficientek.com>
Tested-by: brutser <brutser@perso.be>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
On my laptop running at 2.4GHz, if I run a VM where tsc calibration
using pmtimer will fail presuming a broken pmtimer, it takes ~51 seconds
to do so (as measured with the stopwatch on my phone), with a tsc delta
of 0x1cd1c85300, or around 125 billion cycles.
If instead of trying to wait for 5-200ms to show up on the pmtimer, we
try to wait for 5-200us, it decides it's broken in ~0x2626aa0 TSCs, aka
~2.4 million cycles, or more or less instantly.
Additionally, this reading the pmtimer was returning 0xffffffff anyway,
and that's obviously an invalid return. I've added a check for that and
0 so we don't bother waiting for the test if what we're seeing is dead
pins with no response at all.
If "debug" includes "pmtimer", you will see one of the following three
outcomes. If pmtimer gives all 0 or all 1 bits, you will see:
pmtimer: 0xffffff bad_reads: 1
pmtimer: 0xffffff bad_reads: 2
pmtimer: 0xffffff bad_reads: 3
pmtimer: 0xffffff bad_reads: 4
pmtimer: 0xffffff bad_reads: 5
pmtimer: 0xffffff bad_reads: 6
pmtimer: 0xffffff bad_reads: 7
pmtimer: 0xffffff bad_reads: 8
pmtimer: 0xffffff bad_reads: 9
pmtimer: 0xffffff bad_reads: 10
timer is broken; giving up.
This outcome was tested using qemu+kvm with UEFI (OVMF) firmware and
these options: -machine pc-q35-2.10 -cpu Broadwell-noTSX
If pmtimer gives any other bit patterns but is not actually marching
forward fast enough to use for clock calibration, you will see:
pmtimer delta is 0x0 (1904 iterations)
tsc delta is implausible: 0x2626aa0
This outcome was tested using GRUB patched to not ignore bad reads using
qemu+kvm with UEFI (OVMF) firmware, and these options:
-machine pc-q35-2.10 -cpu Broadwell-noTSX
If pmtimer actually works, you'll see something like:
pmtimer delta is 0xdff
tsc delta is 0x278756
This outcome was tested using qemu+kvm with UEFI (OVMF) firmware, and
these options: -machine pc-i440fx-2.4 -cpu Broadwell-noTSX
I've also tested this outcome on a real Intel Xeon E3-1275v3 on an Intel
Server Board S1200V3RPS using the SDV.RP.B8 "Release" build here:
https://www.intel.com/content/www/us/en/download/674448/firmware-update-for-the-intel-server-board-s1200rp-uefi-development-kit-release-vb8.html
Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
luks2_get_keyslot() can fail for a variety of reasons that do not necessarily
mean the next keyslot should not be tried (e.g. a new kdf type). So always
try the next slot. This will make GRUB more resilient to non-spec json data
that 3rd party systems may add. We do not care if some of the keyslots are
unusable, only if there is at least one that is.
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This command is meant to behave similarly to the "mode" command of the EFI
Shell application. In addition to allowing mode selection by giving the
number of columns and rows as arguments, the command allows specifying the
mode number to select the mode. Also supported are the arguments "min" and
"max", which set the mode to the minimum and maximum mode respectively as
calculated by the columns * rows of that mode.
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
When filesystem detection fails, all that's currently debug-logged is
a series of messages like:
grub-core/kern/fs.c:56:fs: Detecting ntfs...
grub-core/kern/fs.c:76:fs: ntfs detection failed.
repeated for each filesystem. Any messages provided to grub_error() by
the filesystem are lost, and one has to break out gdb to figure out what
went wrong.
With this change, one instead sees:
grub-core/kern/fs.c:56:fs: Detecting fat...
grub-core/osdep/hostdisk.c:357:hostdisk: reusing open device
`/path/to/device'
grub-core/kern/fs.c:77:fs: error: invalid modification timestamp for /.
grub-core/kern/fs.c:79:fs: fat detection failed.
in the debug prints.
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The way the code is written the tofree variable would never be passed to
the free_subchunk() function uninitialized. Coverity cannot determine
this and flags the situation as "Using uninitialized value...". The fix
is just to initialize the local struct.
Fixes: CID 314016
Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Tested-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The EFI_CC_MEASUREMENT_PROTOCOL abstracts the measurement for virtual firmware
in confidential computing environment. It is similar to the EFI_TCG2_PROTOCOL.
It was proposed by Intel and ARM and approved by UEFI organization.
It is defined in Intel GHCI specification: https://cdrdv2.intel.com/v1/dl/getContent/726790 .
The EDKII header file is available at https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Protocol/CcMeasurement.h .
Signed-off-by: Lu Ken <ken.lu@intel.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The event description is a string, so using grub_strcpy() is cleaner than
using grub_memcpy().
Signed-off-by: Lu Ken <ken.lu@intel.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
1. Use macro GRUB_ERR_NONE instead of hard code 0.
2. Keep lowercase of the first char for the status string of log event.
Signed-off-by: Lu Ken <ken.lu@intel.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Using the disk read hook mechanism, setup a read hook on the source disk
which will read from the given header file during the scan and recovery
cryptodisk backend functions. Disk read hooks are executed after the data
has been read from the disk. This is okay, because the read hook is given
the read buffer before its sent back to the caller. In this case, the hook
can then overwrite the data read from the disk device with data from the
header file sent in as the read hook data. This is transparent to the
read caller. Since the callers of this function have just opened the
source disk, there are no current read hooks, so there's no need to
save/restore them nor consider if they should be called or not.
This hook assumes that the header is at the start of the volume, which
is not the case for some formats (e.g. GELI). So GELI will return an
error if a detached header is specified. It also can only be used
with formats where the detached header file can be written to the
first blocks of the volume and the volume could still be unlocked.
So the header file can not be formatted differently from the on-disk
header. If these assumpts are not met, detached header file processing
must be specially handled in the cryptodisk backend module.
The hook will be called potentially many times by a backend. This is fine
because of the assumptions mentioned and the read hook reads from absolute
offsets and is stateless.
Also add a --header (short -H) option to cryptomount which takes a file
argument.
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
It will be desirable in the future to allow having the read hook modify the
data passed back from a read function call on a disk or file. This adds that
infrastructure and has no impact on code flow for existing uses of the read
hook. Also changed is that now when the read hook callback is called it can
also indicate what error code should be sent back to the read caller.
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Adjust the interface of grub_efi_mm_add_regions() to take a set of
GRUB_MM_ADD_REGION_* flags, which most notably is currently only the
GRUB_MM_ADD_REGION_CONSECUTIVE flag. This allows us to set the function
up as callback for the memory subsystem and have it call out to us in
case there's not enough pages available in the current heap.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Patrick Steinhardt <ps@pks.im>
The function add_memory_regions() is currently only called on system
initialization to allocate a fixed amount of pages. As such, it didn't
need to return any errors: in case it failed, we cannot proceed anyway.
This will change with the upcoming support for requesting more memory
from the firmware at runtime, where it doesn't make sense anymore to
fail hard.
Refactor the function to return an error to prepare for this. Note that
this does not change the behaviour when initializing the memory system
because grub_efi_mm_init() knows to call grub_fatal() in case
grub_efi_mm_add_regions() returns an error.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Patrick Steinhardt <ps@pks.im>
In preparation of support for runtime-allocating additional memory
region, this patch extracts the function to retrieve the EFI memory
map and add a subset of it to GRUB's own memory regions.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Patrick Steinhardt <ps@pks.im>
When initializing the EFI memory subsystem, we will by default request
a quarter of the available memory, bounded by a minimum/maximum value.
Given that we're about to extend the EFI memory system to dynamically
request additional pages from the firmware as required, this scaling of
requested memory based on available memory will not make a lot of sense
anymore.
Remove this logic as a preparatory patch such that we'll instead defer
to the runtime memory allocator. Note that ideally, we'd want to change
this after dynamic requesting of pages has been implemented for the EFI
platform. But because we'll need to split up initialization of the
memory subsystem and the request of pages from the firmware, we'd have
to duplicate quite some logic at first only to remove it afterwards
again. This seems quite pointless, so we instead have patches slightly
out of order.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Patrick Steinhardt <ps@pks.im>
Currently, all platforms will set up their heap on initialization of the
platform code. While this works mostly fine, it poses some limitations
on memory management on us. Most notably, allocating big chunks of
memory in the gigabyte range would require us to pre-request this many
bytes from the firmware and add it to the heap from the beginning on
some platforms like EFI. As this isn't needed for most configurations,
it is inefficient and may even negatively impact some usecases when,
e.g., chainloading. Nonetheless, allocating big chunks of memory is
required sometimes, where one example is the upcoming support for the
Argon2 key derival function in LUKS2.
In order to avoid pre-allocating big chunks of memory, this commit
implements a runtime mechanism to add more pages to the system. When
a given allocation cannot be currently satisfied, we'll call a given
callback set up by the platform's own memory management subsystem,
asking it to add a memory area with at least "n" bytes. If this
succeeds, we retry searching for a valid memory region, which should
now succeed.
If this fails, we try asking for "n" bytes, possibly spread across
multiple regions, in hopes that region merging means that we end up
with enough memory for things to work out.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Daniel Axtens <dja@axtens.net>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Patrick Steinhardt <ps@pks.im>
In grub_memalign(), there's a commented section which would allow for
unloading of unneeded modules in case where there is not enough free
memory available to satisfy a request. Given that this code is never
compiled in, let's remove it together with grub_dl_unload_unneeded().
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Patrick Steinhardt <ps@pks.im>
This is handy for debugging. Enable with "set debug=regions".
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Patrick Steinhardt <ps@pks.im>
On x86_64-efi (at least) regions seem to be added from top down. The mm
code will merge a new region with an existing region that comes
immediately before the new region. This allows larger allocations to be
satisfied that would otherwise be the case.
On powerpc-ieee1275, however, regions are added from bottom up. So if
we add 3x 32MB regions, we can still only satisfy a 32MB allocation,
rather than the 96MB allocation we might otherwise be able to satisfy.
* Define 'post_size' as being bytes lost to the end of an allocation
due to being given weird sizes from firmware that are not multiples
of GRUB_MM_ALIGN.
* Allow merging of regions immediately _after_ existing regions, not
just before. As with the other approach, we create an allocated
block to represent the new space and the pass it to grub_free() to
get the metadata right.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Patrick Steinhardt <ps@pks.im>
The corpus was generating issues in grub_btrfs_read_logical() when
attempting to iterate over stripe entries in the superblock's
bootmapping.
In most cases the reason for the failure was that the number of stripes
in chunk->nstripes exceeded the possible space statically allocated in
superblock bootmapping space. Each stripe entry in the bootmapping block
consists of a grub_btrfs_key followed by a grub_btrfs_chunk_stripe.
Another issue that came up was that while calculating the chunk size,
in an earlier piece of code in that function, depending on the data
provided in the btrfs file system, it would end up calculating a size
that was too small to contain even 1 grub_btrfs_chunk_item, which is
obviously invalid too.
Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The fuzzer is generating btrfs file systems that have chunks with
invalid combinations of stripes and substripes for the given RAID
configurations.
After examining the Linux kernel fs/btrfs/tree-checker.c code, it
appears that sub-stripes should only be applied to RAID10, and in that
case there should only ever be 2 of them.
Similarly, RAID single should only have 1 stripe, and RAID1/1C3/1C4
should have 2. 3 or 4 stripes respectively, which is what redundancy
corresponds.
Some of the chunks ended up with a size of 0, which grub_malloc() still
returned memory for and in turn generated ASAN errors later when
accessed.
While it would be possible to specifically limit the number of stripes,
a more correct test was on the combination of the chunk item, and the
number of stripes by the size of the chunk stripe structure in
comparison to the size of the chunk itself.
Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
According to the btrfs code in Linux, the structure of a directory item
leaf should be of the form:
|struct btrfs_dir_item|name|data|
in GRUB the name len and data len are in the grub_btrfs_dir_item
structure's n and m fields respectively.
The combined size of the structure, name and data should be less than
the allocated memory, a difference to the Linux kernel's struct
btrfs_dir_item is that the grub_btrfs_dir_item has an extra field for
where the name is stored, so we adjust for that too.
Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
A corrupt f2fs file system might specify a name length which is greater
than the maximum name length supported by the GRUB f2fs driver.
We will allocate enough memory to store the overly long name, but there
are only F2FS_NAME_LEN bytes in the source, so we would read past the end
of the source.
While checking directory entries, do not copy a file name with an invalid
length.
Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
A corrupt f2fs filesystem could have a block offset or a bitmap
offset that would cause us to read beyond the bounds of the nat
bitmap.
Introduce the nat_bitmap_size member in grub_f2fs_data which holds
the size of nat bitmap.
Set the size when loading the nat bitmap in nat_bitmap_ptr(), and
catch when an invalid offset would create a pointer past the end of
the allocated space.
Check against the bitmap size in grub_f2fs_test_bit() test bit to avoid
reading past the end of the nat bitmap.
Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
A corrupt f2fs file system could specify a nat journal entry count
that is beyond the maximum NAT_JOURNAL_ENTRIES.
Check if the specified nat journal entry count before accessing the
array, and throw an error if it is too large.
Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In a similar vein to the previous patch, parse_line() would write
a NUL byte past the end of the buffer if there was an HTTP header
with a LF rather than a CRLF.
RFC-2616 says:
Many HTTP/1.1 header field values consist of words separated by LWS
or special characters. These special characters MUST be in a quoted
string to be used within a parameter value (as defined in section 3.6).
We don't support quoted sections or continuation lines, etc.
If we see an LF that's not part of a CRLF, bail out.
Fixes: CVE-2022-28734
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
GRUB has special code for handling an http header that is split
across two packets.
The code tracks the end of line by looking for a "\n" byte. The
code for split headers has always advanced the pointer just past the
end of the line, whereas the code that handles unsplit headers does
not advance the pointer. This extra advance causes the length to be
one greater, which breaks an assumption in parse_line(), leading to
it writing a NUL byte one byte past the end of the buffer where we
reconstruct the line from the two packets.
It's conceivable that an attacker controlled set of packets could
cause this to zero out the first byte of the "next" pointer of the
grub_mm_region structure following the current_line buffer.
Do not advance the pointer in the split header case.
Fixes: CVE-2022-28734
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
It's possible for data->sock to get torn down in tcp error handling.
If we unconditionally tear it down again we will end up doing writes
to an offset of the NULL pointer when we go to tear it down again.
Detect if it has been torn down and don't do it again.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Under tftp errors, we print a tftp error message from the tftp header.
However, the tftph pointer is a pointer inside nb, the netbuff. Previously,
we were freeing the nb and then dereferencing it. Don't do that, use it
and then free it later.
This isn't really _bad_ per se, especially as we're single-threaded, but
it trips up fuzzers.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
A malicious tftp server can cause UAFs and a double free.
An attempt to read from a network file is handled by grub_net_fs_read(). If
the read is at an offset other than the current offset, grub_net_seek_real()
is invoked.
In grub_net_seek_real(), if a backwards seek cannot be satisfied from the
currently received packets, and the underlying transport does not provide
a seek method, then grub_net_seek_real() will close and reopen the network
protocol layer.
For tftp, the ->close() call goes to tftp_close() and frees the tftp_data_t
file->data. The file->data pointer is not nulled out after the free.
If the ->open() call fails, the file->data will not be reallocated and will
continue point to a freed memory block. This could happen from a server
refusing to send the requisite ack to the new tftp request, for example.
The seek and the read will then fail, but the grub_file continues to exist:
the failed seek does not necessarily cause the entire file to be thrown
away (e.g. where the file is checked to see if it is gzipped/lzio/xz/etc.,
a read failure is interpreted as a decompressor passing on the file, not as
an invalidation of the entire grub_file_t structure).
This means subsequent attempts to read or seek the file will use the old
file->data after free. Eventually, the file will be close()d again and
file->data will be freed again.
Mark a net_fs file that doesn't reopen as broken. Do not permit read() or
close() on a broken file (seek is not exposed directly to the file API -
it is only called as part of read, so this blocks seeks as well).
As an additional defence, null out the ->data pointer if tftp_open() fails.
That would have lead to a simple null pointer dereference rather than
a mess of UAFs.
This may affect other protocols, I haven't checked.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
I don't really understand what's going on here but fuzzing found
a bug where we read past the end of check_with. That's a C string,
so use grub_strlen() to make sure we don't overread it.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
grub_net_dns_lookup() takes as inputs a pointer to an array of addresses
("addresses") for the given name, and pointer to a number of addresses
("naddresses"). grub_net_dns_lookup() is responsible for allocating
"addresses", and the caller is responsible for freeing it if
"naddresses" > 0.
The DNS recv_hook will sometimes set and free the addresses array,
for example if the packet is too short:
if (ptr + 10 >= nb->tail)
{
if (!*data->naddresses)
grub_free (*data->addresses);
grub_netbuff_free (nb);
return GRUB_ERR_NONE;
}
Later on the nslookup command code unconditionally frees the "addresses"
array. Normally this is fine: the array is either populated with valid
data or is NULL. But in these sorts of error cases it is neither NULL
nor valid and we get a double-free.
Only free "addresses" if "naddresses" > 0.
It looks like the other use of grub_net_dns_lookup() is not affected.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
A netbuff shouldn't be too huge. It's bounded by MTU and TCP segment
reassembly. If we are asked to create one that is unreasonably big, refuse.
This is a hardening measure: if we hit this code, there's a bug somewhere
else that we should catch and fix.
This commit:
- stops the bug propagating any further.
- provides a spot to instrument in e.g. fuzzing to try to catch these bugs.
I have put instrumentation (e.g. __builtin_trap() to force a crash) here and
have not been able to find any more crashes.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
We can receive packets with invalid IP fragmentation information. This
can lead to rsm->total_len underflowing and becoming very large.
Then, in grub_netbuff_alloc(), we add to this very large number, which can
cause it to overflow and wrap back around to a small positive number.
The allocation then succeeds, but the resulting buffer is too small and
subsequent operations can write past the end of the buffer.
Catch the underflow here.
Fixes: CVE-2022-28733
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In some cases attempting to display arbitrary binary strings leads
to ASAN splats reading the widthspec array out of bounds.
Check the index. If it would be out of bounds, return a width of 1.
I don't know if that's strictly correct, but we're not really expecting
great display of arbitrary binary data, and it's certainly not worse than
an OOB read.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Certain 1 px wide images caused a wild pointer write in
grub_jpeg_ycrcb_to_rgb(). This was caused because in grub_jpeg_decode_data(),
we have the following loop:
for (; data->r1 < nr1 && (!data->dri || rst);
data->r1++, data->bitmap_ptr += (vb * data->image_width - hb * nc1) * 3)
We did not check if vb * width >= hb * nc1.
On a 64-bit platform, if that turns out to be negative, it will underflow,
be interpreted as unsigned 64-bit, then be added to the 64-bit pointer, so
we see data->bitmap_ptr jump, e.g.:
0x6180_0000_0480 to
0x6181_0000_0498
^
~--- carry has occurred and this pointer is now far away from
any object.
On a 32-bit platform, it will decrement the pointer, creating a pointer
that won't crash but will overwrite random data.
Catch the underflow and error out.
Fixes: CVE-2021-3697
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
An invalid file could contain multiple start of stream blocks, which
would cause us to reallocate and leak our bitmap. Refuse to handle
multiple start of streams.
Additionally, fix a grub_error() call formatting.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Fix a memory leak where an invalid file could cause us to reallocate
memory for a huffman table we had already allocated memory for.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Fuzzing revealed some inputs that were taking a long time, potentially
forever, because they did not bail quickly upon encountering an I/O error.
Try to catch I/O errors sooner and bail out.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
ASAN picked up two OOB global reads: we weren't checking if some code
values fit within the cplens or cpdext arrays. Check and throw an error
if not.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In fuzzing we observed crashes where a code would attempt to be inserted
into a huffman table before the start, leading to a set of heap OOB reads
and writes as table entries with negative indices were shifted around and
the new code written in.
Catch the case where we would underflow the array and bail.
Fixes: CVE-2021-3696
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
A 16-bit greyscale PNG without alpha is processed in the following loop:
for (i = 0; i < (data->image_width * data->image_height);
i++, d1 += 4, d2 += 2)
{
d1[R3] = d2[1];
d1[G3] = d2[1];
d1[B3] = d2[1];
}
The increment of d1 is wrong. d1 is incremented by 4 bytes per iteration,
but there are only 3 bytes allocated for storage. This means that image
data will overwrite somewhat-attacker-controlled parts of memory - 3 bytes
out of every 4 following the end of the image.
This has existed since greyscale support was added in 2013 in commit
3ccf16dff98f (grub-core/video/readers/png.c: Support grayscale).
Saving starfield.png as a 16-bit greyscale image without alpha in the gimp
and attempting to load it causes grub-emu to crash - I don't think this code
has ever worked.
Delete all PNG greyscale support.
Fixes: CVE-2021-3695
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This causes the bitmap to be leaked. Do not permit multiple image headers.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Fuzzing revealed some inputs that were taking a long time, potentially
forever, because they did not bail quickly upon encountering an I/O error.
Try to catch I/O errors sooner and bail out.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
If we have an error in grub_file_open() before we free device_name, we
will leak it.
Free device_name in the error path and null out the pointer in the good
path once we free it there.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
We must not allow other verifiers to pass things like the GRUB modules.
Instead of maintaining a blocklist, maintain an allowlist of things
that we do not care about.
This allowlist really should be made reusable, and shared by the
lockdown verifier, but this is the minimal patch addressing
security concerns where the TPM verifier was able to mark modules
as verified (or the OpenPGP verifier for that matter), when it
should not do so on shim-powered secure boot systems.
Fixes: CVE-2022-28735
Signed-off-by: Julian Andres Klode <julian.klode@canonical.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
