Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

skip empty lines in entry file #185

Open
wants to merge 367 commits into
base: fedora-41
Choose a base branch
from

Conversation

teknoraver
Copy link

grub_file_getline() doesn't make any distinction between empty lines or EOF, so the following entry:

title Fedora Linux (6.10.0-matteo) 40 (KDE Plasma)
version 6.10.0-matteo
linux /boot/vmlinuz-6.10.0-matteo

options root=/dev/nvme0n1p2 ro cgroup_no_v1=all

boots the kernel with an empty command line.
Fix this by also checking grub_errno, which correctly signals the EOF.

David Abdurachmanov and others added 30 commits February 6, 2023 17:33
All other architectures have start symbol.

Hopefully this resolves:

    BUILDSTDERR: ././grub-mkimage: error: undefined symbol start.

Signed-off-by: David Abdurachmanov <[email protected]>
The python-unversioned-command package is not installed in the buildroot,
but the bootstrap script expects the python command to be present if one
is not defined. So building the package leads to the following error:

./autogen.sh: line 20: python: command not found

This is harmless since gnulib is included as a source anyways, because the
builders can't download. But still the issue should be fixed by forcing to
use python3 that's the default in Fedora now.

Signed-off-by: Javier Martinez Canillas <[email protected]>
The fw_path environment variable is used by http_configure() function to
determine the HTTP path that should be used as prefix when using relative
HTTP paths. And this is stored in the http_path environment variable.

Later, that variable is looked up by grub_efihttp_open() to generate the
complete path to be used in the HTTP request.

But these variables are not exported, which means that are not global and
so are only found in the initial context.

This can cause commands like configfile that create a new context to fail
because the fw_path and http_path variables will not be found.

Resolves: rhbz#1616395

Signed-off-by: Javier Martinez Canillas <[email protected]>
According to RFC 2732 (https://www.ietf.org/rfc/rfc2732.txt), literal IPv6
addresses must be enclosed in square brackets. But GRUB currently does not
do this and is causing HTTP servers to send Bad Request (400) responses.

For example, the following is the HTTP stream when fetching a config file:

HEAD /EFI/BOOT/grub.cfg HTTP/1.1
Host: 2000:dead:beef:a::1
Accept: */*
User-Agent: UefiHttpBoot/1.0

HTTP/1.1 400 Bad Request
Date: Thu, 05 Mar 2020 14:46:02 GMT
Server: Apache/2.4.41 (Fedora) OpenSSL/1.1.1d
Connection: close
Content-Type: text/html; charset=iso-8859-1

and after enclosing the IPv6 address the HTTP request is successful:

HEAD /EFI/BOOT/grub.cfg HTTP/1.1
Host: [2000:dead:beef:a::1]
Accept: */*
User-Agent: UefiHttpBoot/1.0

HTTP/1.1 200 OK
Date: Thu, 05 Mar 2020 14:48:04 GMT
Server: Apache/2.4.41 (Fedora) OpenSSL/1.1.1d
Last-Modified: Thu, 27 Feb 2020 17:45:58 GMT
ETag: "206-59f924b24b1da"
Accept-Ranges: bytes
Content-Length: 518

Resolves: rhbz#1732765

Signed-off-by: Javier Martinez Canillas <[email protected]>
The grub_efi_net_parse_address() function is not covering the case where a
port number is specified in an IPv4 or IPv6 address, so will fail to parse
the network address.

For most cases the issue is harmless, because the function is only used to
match an address with a network interface and if fails the default is used.

But still is a bug that has to be fixed and it causes error messages to be
printed like the following:

error: net/efi/net.c:782:unrecognised network address '192.168.122.1:8080'

error: net/efi/net.c:781:unrecognised network address '[2000:dead:beef:a::1]:8080'

Resolves: rhbz#1732765

Signed-off-by: Javier Martinez Canillas <[email protected]>
The grub_efi_string_to_ip4_address() function wrongly assumes that an IPv6
address is an IPv4 address, because it doesn't take into account the case
of a caller passing an IPv6 address as a string.

This leads to the grub_efi_net_parse_address() function to fail and print
the following error message:

error: net/efi/net.c:785:unrecognised network address '2000:dead:beef:a::1'

Resolves: rhbz#1732765

Signed-off-by: Javier Martinez Canillas <[email protected]>
Currently if parsing the address fails an error message is printed. But in
most cases this isn't a fatal error since the grub_efi_net_parse_address()
function is only used to match an address with a network interface to use.

And if this fails, the default interface is used which is good enough for
most cases. So instead of printing an error that would pollute the console
just print a debug message if the address is not parsed correctly.

A user can enable debug messages for the efinet driver to have information
about the failure and the fact that the default interface is being used.

Related: rhbz#1732765

Signed-off-by: Javier Martinez Canillas <[email protected]>
Make F8, which used to be the hotkey to show the Windows boot menu during
boot for a long long time, also interrupt sleeps / stop the menu countdown.

Signed-off-by: Javier Martinez Canillas <[email protected]>
Upstream GRUB uses the EFI LoadImage() and StartImage() to boot the Linux
kernel. But our custom EFI loader that supports Secure Boot instead uses
the EFI handover protocol (for x86) or jumping directly to the PE/COFF
entry point (for aarch64).

This is done to allow the bootloader to verify the images using the shim
lock protocol to avoid booting untrusted binaries.

Since the bootloader loads the kernel from the boot media instead of using
LoadImage(), it is responsible to set the Loaded Image base address before
booting the kernel.

Otherwise the kernel EFI stub will complain that it was not set correctly
and print the following warning message:

EFI stub: ERROR: FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value

Resolves: rhbz#1814690

Signed-off-by: Javier Martinez Canillas <[email protected]>
Currently if the EFI firmware fails to do a TPM measurement for a file,
the error will be propagated to the verifiers framework and so opening
the file will not succeed.

This mean that buggy firmwares will prevent the system to boot since the
loader won't be able to open any file. But failing to do TPM measurements
shouldn't be a fatal error and the system should still be able to boot.

Signed-off-by: Javier Martinez Canillas <[email protected]>
The EFI linux loader allocates a bounce buffer to copy the initrd since in
some machines doing DMA on addresses above 4GB is not possible during EFI.

But the verifiers framework also allocates a buffer to copy the initrd in
its grub_file_open() handler. It does this since the data to verify has to
be passed as a single chunk to modules that use the verifiers framework.

If the initrd image size is big there may not be enough memory in the heap
to allocate two buffers of that size. This causes an allocation failure in
the verifiers framework and leads to the initrd not being read.

To prevent these allocation failures, let's reduce the maximum size of the
bounce buffer used in the EFI loader. Since the data read can be copied to
the actual initrd address in multilple chunks.

Resolves: rhbz#1838633

Signed-off-by: Javier Martinez Canillas <[email protected]>
There are two different HTTP drivers that can be used when requesting an
HTTP resource: the efi/http that uses the EFI_HTTP_PROTOCOL and the http
that uses GRUB's HTTP and TCP/IP implementation.

The efi/http driver appends a prefix that is defined in the variable
http_path, but the http driver doesn't.

So using this driver and attempting to fetch a resource using a relative
path fails.

Signed-off-by: Javier Martinez Canillas <[email protected]>
Somewhere along the way this got mis-merged to include a return without
a value.  Fix it up.

Signed-off-by: Peter Jones <[email protected]>
In theory all of this data comes from the firmware stack and it should
be safe, but it's better to be paranoid.

Signed-off-by: Peter Jones <[email protected]>
These could be triggered by an extremely large number of arguments to
the initrd command on 32-bit architectures, or a crafted filesystem with
very large files on any architecture.

Signed-off-by: Colin Watson <[email protected]>
If certificates that signed grub are installed into db, grub can be
booted directly. It will then boot any kernel without signature
validation. The booted kernel will think it was booted in secureboot
mode and will implement lockdown, yet it could have been tampered.

This version of the patch skips calling verification, when booted
without secureboot. And is indented with gnu ident.

CVE-2020-15705

Reported-by: Mathieu Trudel-Lapierre <[email protected]>
Signed-off-by: Dimitri John Ledkov <[email protected]>
This will need to get folded back in the right place on the next rebase,
but it's before "Make grub_strtol() "end" pointers have safer const
qualifiers" currently, so for now I'm leaving it here instead of merging
it back with the original patch.

Signed-off-by: Peter Jones <[email protected]>
This will need to get folded back in the right place on the next rebase,
but it's before "Make grub_strtol() "end" pointers have safer const
qualifiers" currently, so for now I'm leaving it here instead of merging
it back with the original patch.

Signed-off-by: Peter Jones <[email protected]>
This will need to get folded back in the right place on the next rebase,
but it's before "Make grub_strtol() "end" pointers have safer const
qualifiers" currently, so for now I'm leaving it here instead of merging
it back with the original patch.

Signed-off-by: Peter Jones <[email protected]>
This will need to get folded back in the right place on the next rebase,
but it's before "Make grub_strtol() "end" pointers have safer const
qualifiers" currently, so for now I'm leaving it here instead of merging
it back with the original patch.

Signed-off-by: Peter Jones <[email protected]>
This will need to get folded back in the right place on the next rebase,
but it's before "Make grub_strtol() "end" pointers have safer const
qualifiers" currently, so for now I'm leaving it here instead of merging
it back with the original patch.

Signed-off-by: Peter Jones <[email protected]>
…er-menu=xxx" work with grub

This commit adds a number of scripts / config files to make
"systemctl reboot --boot-loader-menu=xxx" work with grub:

1. /lib/systemd/system/systemd-logind.service.d/10-grub.conf
This sets SYSTEMD_REBOOT_TO_BOOT_LOADER_MENU in the env. for logind,
indicating that the boot-loader which is used supports this feature, see:
https://github.com/systemd/systemd/blob/master/docs/ENVIRONMENT.md

2. /lib/systemd/system/grub-systemd-integration.service
   /lib/systemd/system/reboot.target.wants/grub-systemd-integration.service ->
     ../grub-systemd-integration.service
   /usr/libexec/grub/grub-systemd-integration.sh

The symlink in the .wants dir causes the added service file to be started
by systemd just before rebooting the system.
If /run/systemd/reboot-to-boot-loader-menu exist then the service will run
the grub-systemd-integration.sh script.
This script sets the new menu_show_once_timeout grubenv variable to the
requested timeout in seconds.

3. /etc/grub.d/14_menu_show_once

This new grub-mkconfig snippet adds the necessary code to the generated
grub.conf to honor the new menu_show_once_timeout variable, and to
automatically clear it after consuming it.

Note the service and libexec script use grub-systemd-integration as name
because in the future they may be used to add further integration with
systemctl reboot --foo options, e.g. support for --boot-loader-entry=NAME.

A few notes about upstreaming this patch from the rhboot grub2 fork:
1. I have deliberately put the grub.conf bits for this in a new / separate
   grub-mkconfig snippet generator for easy upstreaming
2. Even though the commit message mentions the .wants symlink for the .service
   I have been unable to come up with a clean way to do this at "make install"
   time, this should be fixed before upstreaming.

Downstream notes:
1. Since make install does not add the .wants symlink, this needs to be done
   in grub2.spec %install
2. This is keeping support for the "old" Fedora specific menu_show_once env
   variable, which has a hardcoded timeout of 60 sec in 12_menu_auto_hide in
   place for now. This can be dropped (eventually) in a follow-up patch once
   GNOME has been converted to use the systemd dbus API equivalent of
   "systemctl reboot --boot-loader-menu=xxx".

Signed-off-by: Hans de Goede <[email protected]>
Downstream RH / Fedora patch for compatibility with old, not (yet)
regenerated grub.cfg files which miss the menu_show_once_timeout check.
This older grubenv variable leads to a fixed timeout of 60 seconds.

Note that the new menu_show_once_timeout will overrule these 60 seconds
if both are set and the grub.cfg does have the menu_show_once_timeout
check.

Signed-off-by: Hans de Goede <[email protected]>
When keyboard controller acts in Translate mode (0x40 mask), then use
set 1 since translation is done.
Otherwise use the mode queried from the controller (usually set 2).

Added "atkeyb" debugging messages in at_keyboard module as well.

Resolves: rhbz#1897587

Tested on:
- Asus N53SN (set 1 used)
- Dell Precision (set 1 used)
- HP Elitebook (set 2 used)
- HP G5430 (set 1 used, keyboard in XT mode!)
- Lenovo P71 & Lenovo T460s (set 2 used)
- QEMU/KVM (set 1 used)

Signed-off-by: Renaud Métrich <[email protected]>
For each platform, GRUB is shipped as a kernel image and a set of
modules. These files are then used by the grub-install utility to
install GRUB on a specific device. However, in order to support UEFI
Secure Boot, the resulting EFI binary must be signed by a recognized
private key. For this reason, for EFI platforms, most distributions also
ship prebuilt EFI binaries signed by a distribution-specific private
key. In this case, however, the grub-install utility should not be used
because it would overwrite the signed EFI binary.

The current fix is suboptimal because it preserves all EFI-related code.
A better solution could be to modularize the code and provide a
build-time option.

Resolves: rhbz#1737444

Signed-off-by: Jan Hlavac <[email protected]>
[rharwood: drop man page]
…th absolute and relative timestamp

Signed-off-by: Renaud Métrich <[email protected]>
… is in the environment and not empty

Signed-off-by: Renaud Métrich <[email protected]>
ElyesH and others added 26 commits November 30, 2023 12:49
Signed-off-by: Elyes Haouas <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
Signed-off-by: t.feng <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
While performing fuzz testing with XFS filesystem images with ASAN
enabled, several issues were found where the memory accesses are made
beyond the data that is allocated into the struct grub_xfs_data
structure's data field.

The existing structure didn't store the size of the memory allocated into
the buffer in the data field and had no way to check it. To resolve these
issues, the data size is stored to enable checks into the data buffer.

With these checks in place, the fuzzing corpus no longer cause any crashes.

Signed-off-by: Darren Kenny <[email protected]>
Signed-off-by: Robbie Harwood <[email protected]>
Signed-off-by: Marta Lewandowska <[email protected]>
Signed-off-by: Lidong Chen <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
After parsing of the current entry, the entry pointer is advanced
to the next entry at the end of the "for" loop. In case where the
last entry is at the end of the data boundary, the advanced entry
pointer can point off the data boundary. The subsequent boundary
check for the advanced entry pointer can cause a failure.

The fix is to include the boundary check into the "for" loop
condition.

Signed-off-by: Lidong Chen <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
Tested-by: Sebastian Andrzej Siewior <[email protected]>
Tested-by: Marta Lewandowska <[email protected]>
The XFS directory entry parsing code has never been completely correct
for extent based directories. The parser correctly handles the case
where the directory is contained in a single extent, but then mistakenly
assumes the data blocks for the multiple extent case are each identical
to the single extent case. The difference in the format of the data
blocks between the two cases is tiny enough that its gone unnoticed for
a very long time.

A recent change introduced some additional bounds checking into the XFS
parser. Like GRUB's existing parser, it is correct for the single extent
case but incorrect for the multiple extent case. When parsing a directory
with multiple extents, this new bounds checking is sometimes (but not
always) tripped and triggers an "invalid XFS directory entry" error. This
probably would have continued to go unnoticed but the /boot/grub/<arch>
directory is large enough that it often has multiple extents.

The difference between the two cases is that when there are multiple
extents, the data blocks do not contain a trailer nor do they contain
any leaf information. That information is stored in a separate set of
extents dedicated to just the leaf information. These extents come after
the directory entry extents and are not included in the inode size. So
the existing parser already ignores the leaf extents.

The only reason to read the trailer/leaf information at all is so that
the parser can avoid misinterpreting that data as directory entries. So
this updates the parser as follows:

For the single extent case the parser doesn't change much:
1. Read the size of the leaf information from the trailer
2. Set the end pointer for the parser to the start of the leaf
   information. (The previous bounds checking set the end pointer to the
   start of the trailer, so this is actually a small improvement.)
3. Set the entries variable to the expected number of directory entries.

For the multiple extent case:
1. Set the end pointer to the end of the block.
2. Do not set up the entries variable. Figuring out how many entries are
   in each individual block is complex and does not seem worth it when
   it appears to be safe to just iterate over the entire block.

The bounds check itself was also dependent upon the faulty XFS parser
because it accidentally used "filename + length - 1". Presumably this
was able to pass the fuzzer because in the old parser there was always
8 bytes of slack space between the tail pointer and the actual end of
the block. Since this is no longer the case the bounds check needs to be
updated to "filename + length + 1" in order to prevent a regression in
the handling of corrupt fliesystems.

Notes:
* When there is only one extent there will only ever be one block. If
  more than one block is required then XFS will always switch to holding
  leaf information in a separate extent.
* B-tree based directories seems to be parsed properly by the same code
  that handles multiple extents. This is unlikely to ever occur within
  /boot though because its only used when there are an extremely large
  number of directory entries.

Fixes: ef7850c (fs/xfs: Fix issues found while fuzzing the XFS filesystem)
Fixes: b2499b2 (Adds support for the XFS filesystem.)
Fixes: https://savannah.gnu.org/bugs/?64376

Signed-off-by: Jon DeVree <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
Tested-by: Sebastian Andrzej Siewior <[email protected]>
Tested-by: Marta Lewandowska <[email protected]>
XFS introduced 64-bit extent counters for inodes via a series of
upstream commits and the feature was marked as stable in v6.5 via
commit 61d7e8274cd8 (xfs: drop EXPERIMENTAL tag for large extent
counts).

Further, xfsprogs release v6.5.0 switched this feature on by default
in mkfs.xfs via commit e5b18d7d1d96 (mkfs: enable large extent counts
by default).

Filesystems formatted with large extent count support, nrext64=1, are
thus currently not recognizable by GRUB, since this is an incompat
feature. Add the required support so that those filesystems and inodes
with large extent counters can be read by GRUB.

Signed-off-by: Anthony Iliopoulos <[email protected]>
Reviewed-by: Andrey Albershteyn <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
Tested-by: Marta Lewandowska <[email protected]>
Tested-by: Sebastian Andrzej Siewior <[email protected]>
Remove the debug message "/EndEntire" while using GRUB chainloader command.

Signed-off-by: raravind  <[email protected]>
(cherry picked from commit f75f538)
Following up on CVE-2019-14865 and taking a fresh look at
grub2-set-bootflag now (through my work at CIQ on Rocky Linux), I saw some
other ways in which users could still abuse this little program:

1. After CVE-2019-14865 fix, grub2-set-bootflag no longer rewrites the
grubenv file in-place, but writes into a temporary file and renames it
over the original, checking for error returns from each call first.
This prevents the original file truncation vulnerability, but it can
leave the temporary file around if the program is killed before it can
rename or remove the file.  There are still many ways to get the program
killed, such as through RLIMIT_FSIZE triggering SIGXFSZ (tested,
reliable) or by careful timing (tricky) of signals sent by process group
leader, pty, pre-scheduled timers, SIGXCPU (probably not an exhaustive
list).  Invoking the program multiple times fills up /boot (or if /boot
is not separate, then it can fill up the root filesystem).  Since the
files are tiny, the filesystem is likely to run out of free inodes
before it'd run out of blocks, but the effect is similar - can't create
new files after this point (but still can add data to existing files,
such as logs).

2. After CVE-2019-14865 fix, grub2-set-bootflag naively tries to protect
itself from signals by becoming full root.  (This does protect it from
signals sent by the user directly to the PID, but e.g. "kill -9 -1" by
the user still works.)  A side effect of such "protection" is that it's
possible to invoke more concurrent instances of grub2-set-bootflag than
the user's RLIMIT_NPROC would normally permit (as specified e.g. in
/etc/security/limits.conf, or say in Apache httpd's RLimitNPROC if
grub2-set-bootflag would be abused by a website script), thereby
exhausting system resources (e.g., bypassing RAM usage limit if
RLIMIT_AS was also set).

3. umask is inherited.  Again, due to how the CVE-2019-14865 fix creates
a new file, and due to how mkstemp() works, this affects grubenv's new
file permissions.  Luckily, mkstemp() forces them to be no more relaxed
than 0600, but the user ends up being able to set them e.g. to 0.
Luckily, at least in my testing GRUB still works fine even when the file
has such (lack of) permissions.

This commit deals with the abuses above as follows:

1. RLIMIT_FSIZE is pre-checked, so this specific way to get the process
killed should no longer work.  However, this isn't a complete fix
because there are other ways to get the process killed after it has
created the temporary file.

The commit also fixes bug 1975892 ("RFE: grub2-set-bootflag should not
write the grubenv when the flag being written is already set") and
similar for "menu_show_once", which further reduces the abuse potential.

2. RLIMIT_NPROC bypass should be avoided by not becoming full root (aka
dropping the partial "kill protection").

3. A safe umask is set.

This is a partial fix (temporary files can still accumulate, but this is
harder to trigger).

While at it, this commit also fixes potential 1- or 2-byte over-read of
env[] if its content is malformed - this was not a security issue since the
grubenv file is trusted input, and the fix is just for robustness.

Signed-off-by: Solar Designer <[email protected]>
Switch to per-user fixed temporary filenames along with a weird locking
mechanism, which is explained in source code comments.  This is a more
complete fix than the previous commit (temporary files can't accumulate).
Unfortunately, it introduces new risks (by working on a temporary file
shared between the user's invocations), which are _hopefully_ avoided by
the patch's elaborate logic.  I actually got it wrong at first, which
suggests that this logic is hard to reason about, and more errors or
omissions are possible.  It also relies on the kernel's primitives' exact
semantics to a greater extent (nothing out of the ordinary, though).

Remaining issues that I think cannot reasonably be fixed without a
redesign (e.g., having per-flag files with nothing else in them) and
without introducing new issues:

A. A user can still revert a concurrent user's attempt of setting the
other flag - or of making other changes to grubenv by means other than
this program.

B. One leftover temporary file per user is still possible.

Signed-off-by: Solar Designer <[email protected]>
Exit calmly when not installed SUID root and invoked by non-root.  This
allows installing user/grub-boot-success.service unconditionally while
supporting non-SUID installation of the program for some limited usage.

Signed-off-by: Solar Designer <[email protected]>
…for the $MFT file

When parsing an extremely fragmented $MFT file, i.e., the file described
using the $ATTRIBUTE_LIST attribute, current NTFS code will reuse a buffer
containing bytes read from the underlying drive to store sector numbers,
which are consumed later to read data from these sectors into another buffer.

These sectors numbers, two 32-bit integers, are always stored at predefined
offsets, 0x10 and 0x14, relative to first byte of the selected entry within
the $ATTRIBUTE_LIST attribute. Usually, this won't cause any problem.

However, when parsing a specially-crafted file system image, this may cause
the NTFS code to write these integers beyond the buffer boundary, likely
causing the GRUB memory allocator to misbehave or fail. These integers contain
values which are controlled by on-disk structures of the NTFS file system.

Such modification and resulting misbehavior may touch a memory range not
assigned to the GRUB and owned by firmware or another EFI application/driver.

This fix introduces checks to ensure that these sector numbers are never
written beyond the boundary.

Fixes: CVE-2023-4692

Reported-by: Maxim Suhanov <[email protected]>
Signed-off-by: Maxim Suhanov <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
…tribute

When reading a file containing resident data, i.e., the file data is stored in
the $DATA attribute within the NTFS file record, not in external clusters,
there are no checks that this resident data actually fits the corresponding
file record segment.

When parsing a specially-crafted file system image, the current NTFS code will
read the file data from an arbitrary, attacker-chosen memory offset and of
arbitrary, attacker-chosen length.

This allows an attacker to display arbitrary chunks of memory, which could
contain sensitive information like password hashes or even plain-text,
obfuscated passwords from BS EFI variables.

This fix implements a check to ensure that resident data is read from the
corresponding file record segment only.

Fixes: CVE-2023-4693

Reported-by: Maxim Suhanov <[email protected]>
Signed-off-by: Maxim Suhanov <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
… and non-resident index attributes

This fix introduces checks to ensure that index entries are never read
beyond the corresponding directory index.

The lack of this check is a minor issue, likely not exploitable in any way.

Reported-by: Maxim Suhanov <[email protected]>
Signed-off-by: Maxim Suhanov <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
This fix introduces checks to ensure that bitmaps for directory indices
are never read beyond their actual sizes.

The lack of this check is a minor issue, likely not exploitable in any way.

Reported-by: Maxim Suhanov <[email protected]>
Signed-off-by: Maxim Suhanov <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
This fix introduces checks to ensure that an NTFS volume label is always
read from the corresponding file record segment.

The current NTFS code allows the volume label string to be read from an
arbitrary, attacker-chosen memory location. However, the bytes read are
always treated as UTF-16LE. So, the final string displayed is mostly
unreadable and it can't be easily converted back to raw bytes.

The lack of this check is a minor issue, likely not causing a significant
data leak.

Reported-by: Maxim Suhanov <[email protected]>
Signed-off-by: Maxim Suhanov <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
Move some calls used to access NTFS attribute header fields into
functions with human-readable names.

Suggested-by: Daniel Kiper <[email protected]>
Signed-off-by: Maxim Suhanov <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
Commit de735a4 added a grub_env_set where the prefix contains
the arch name in the pathname. This create issues when trying to
load modules using this prefix as the pathname contains a "doubled"
arch name in it (ie .../powerpc-ieee1275/powerpc-ieee1275/).

Signed-off-by: Nicolas Frayer <[email protected]>
Stricker permissions are required on the grub.cfg file, resulting in
at most 0600 owner's file permissions. This resolves conflicting
requirement permissions on grub2-pc package's grub2.cfg file.

Signed-off-by: Leo Sandoval <[email protected]>
The directory extent list does not have to be a continuous list of data
blocks. When GRUB tries to read a non-existant member of the list,
grub_xfs_read_file() will return a block of zero'ed memory. Checking for
a zero'ed magic number is sufficient to skip this non-existant data block.

Prior to commit 07318ee (fs/xfs: Fix XFS directory extent parsing)
this was handled as a subtle side effect of reading the (non-existant)
tail data structure. Since the block was zero'ed the computation of the
number of directory entries in the block would return 0 as well.

Fixes: 07318ee (fs/xfs: Fix XFS directory extent parsing)
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2254370

Signed-off-by: Jon DeVree <[email protected]>
Reviewed-By: Vladimir Serbinenko <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
fixes bz#2223437

Signed-off-by: Marta Lewandowska <[email protected]>
The initial fix implemented a new flag that forces the grub cfg
stub to be located on the same disk as grub. This created several
issues such as RAID machines not being able to boot as their
partition names under grub were different from the partition where
grub is located. It also simply means that any machines with the
/boot partition located on a disk other than the one containing grub
won't boot.
This commit denies booting if the grub cfg stub is located on a USB
drive with a duplicated UUID (UUID being the same as the partition
containing the actual grub cfg stub)

Signed-off-by: Nicolas Frayer <[email protected]>
For those manual assembly files created where no '.note.GNU-stack'
section is explicitly added, linker defaults it as executable and this
is the reason that RHEL CI rpminspect & annocheck tests are
failing. The proposed change sets the corresponding GNU-stack
sections otherwise CI detects the following security vulnerability

   $ annocheck annocheck --ignore-unknown --verbose --profile=el9 *.rpm 2>&1 | grep FAIL | grep stack
   (standard input):(standard input):Hardened: ./usr/lib/grub/x86_64-efi/kernel.exec: FAIL: gnu-stack test because .note.GNU-stack section has execute permission
   (standard input):(standard input):Hardened: ./usr/lib/grub/x86_64-efi/kernel.img: FAIL: gnu-stack test because .note.GNU-stack section has execute permission

Signed-off-by: Leo Sandoval <[email protected]>
By mistake, PR [1] changed the permission bits to 755 so revert it to
644 again.

[1] rhboot#167

Signed-off-by: Leo Sandoval <[email protected]>
This patch adds support for Radix, Xive and Radix_gtse in Options
vector5 which is required for KVM LPARs. KVM LPARs ONLY support
Radix and not the Hash. Not enabling Radix on any PowerVM KVM LPARs
will result in boot failure.

Signed-off-by: Avnish Chouhan <[email protected]>
Reviewed-by: Daniel Kiper <[email protected]>
/boot/efi/EFI/$os_name/grub.cfg contains a grub cfg stub
that should not be overwritten by grub2-mkconfig.
Ensure that we prevent this from happening.

Signed-off-by: Marta Lewandowska <[email protected]>
Signed-off-by: Nicolas Frayer <[email protected]>
grub_file_getline() doesn't make any distinction between empty lines or EOF,
so the following entry:

    title Fedora Linux (6.10.0-matteo) 40 (KDE Plasma)
    version 6.10.0-matteo
    linux /boot/vmlinuz-6.10.0-matteo

    options root=/dev/nvme0n1p2 ro cgroup_no_v1=all

boots the kernel with an empty command line.
Fix this by also checking grub_errno, which correctly signals the EOF.

Signed-off-by: Matteo Croce <[email protected]>
@@ -538,7 +538,7 @@ static int read_entry (
char *separator;

buf = grub_file_getline (f);
if (!buf)
if (!buf && grub_errno)
Copy link

@lsandov1 lsandov1 Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Matteo,

help me to undestand: does the function grub_file_getline set grub_errno in case of EOF?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not directly, but it calls grub_file_read which should.
I see the same pattern here:
https://github.com/rhboot/grub2/blob/fedora-41/grub-core/commands/legacycfg.c#L81-L84

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try to reproduce the issue with the entry in the commit message?
Anyway, I don't know who designed grub_file_getline in a way which is impossible to distinguish between EOF and an empty line.
There are 14 usages for grub_file_getline, that could be changed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a pretty old change though: 2cff367

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know it if breaks other usages, what do you think about a change like 398f346?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@teknoraver thanks for the explanation. I have added other reviewers . Also, if possible try sending the same patch but rebased for grub2-2.12 into the grub-devel mailing list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.