In grub-core/fs/jfs.c, Coverity spotted redundant code where the pointer diro
was being set to 0 and then being overwritten later without being used. Since
this is unnecessary, we can remove the code that sets diro to 0.
Fixes: CID 428876
Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
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 <ailiop@suse.com>
Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Marta Lewandowska <mlewando@redhat.com>
Tested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
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: ef7850c75 (fs/xfs: Fix issues found while fuzzing the XFS filesystem)
Fixes: b2499b29c (Adds support for the XFS filesystem.)
Fixes: https://savannah.gnu.org/bugs/?64376
Signed-off-by: Jon DeVree <nuxi@vault24.org>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Tested-by: Marta Lewandowska <mlewando@redhat.com>
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 <lidong.chen@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Tested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Tested-by: Marta Lewandowska <mlewando@redhat.com>
Original commit is wrong because grub_file_get_device_name() may return NULL
if we use implicit $root. Additionally, the grub_errno is guaranteed to be
GRUB_ERR_NONE at the beginning of a command. So, everything should work as
expected and Coverity report, CID 73668, WRT to this code should be treated
as false positive.
This reverts commit 7aab03418 (zfsinfo: Correct a check for error allocating memory).
Fixes: 7aab03418 (zfsinfo: Correct a check for error allocating memory)
Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Implicit holes in file data need to be zeroed explicitly, instead of
just leaving the data in the buffer uninitialized.
This led to kernels randomly failing to boot in "fun" ways when loaded
from btrfs with the no_holes feature enabled, because large blocks of
zeros in the kernel file contained random data instead.
Signed-off-by: Fabian Vogt <fvogt@suse.de>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Move some calls used to access NTFS attribute header fields into
functions with human-readable names.
Suggested-by: Daniel Kiper <daniel.kiper@oracle.com>
Signed-off-by: Maxim Suhanov <dfirblog@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
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 <dfirblog@gmail.com>
Signed-off-by: Maxim Suhanov <dfirblog@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
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 <dfirblog@gmail.com>
Signed-off-by: Maxim Suhanov <dfirblog@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
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 <dfirblog@gmail.com>
Signed-off-by: Maxim Suhanov <dfirblog@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
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 <dfirblog@gmail.com>
Signed-off-by: Maxim Suhanov <dfirblog@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
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 <dfirblog@gmail.com>
Signed-off-by: Maxim Suhanov <dfirblog@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Some dnodes are shared with properties zap. This is used
e.g. for quotas. Then dnode type is 0xc4 and GRUB stumbles on
this. Check bonus type and if it's ok then ignore dnode type mismatch
Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Reading them is harmless but useless as they are empty by definition
Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
We ended up comparing over unset values as we had dnode_phys on one side
and dnode on another
Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
This is a speedup used in some ZFS version. This trips GRUB and makes it
unable to access directories. Just skip it for now and revisit
if we ever need this speedup.
Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Specifically, return GRUB_ERR_BAD_FILE_TYPE because this is what is
expected by the ls command when it is given a path to a non-directory.
This fixes a bug where calling ls with a list of non-directory paths
outputs a blank line for each such argument.
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
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 <darren.kenny@oracle.com>
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
Signed-off-by: Marta Lewandowska <mlewando@redhat.com>
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
When an invalid node size is detected in grub_hfsplus_mount(), data
pointer is freed. Thus, file->data is not set. The code should also
set the grub_errno when that happens to indicate an error and to avoid
accessing the uninitialized file->data in grub_file_close().
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The SUSP specs demand that the reading of the next SUSP area which is
depicted by a CE entry shall be delayed until reading of the current
SUSP area is completed. Up to now GRUB immediately ends reading of the
current area and loads the new one. So, buffer the parameters of a found
CE entry and perform checks and reading of new data only after the
reader loop has ended.
Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>
Tested-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
If processing of a SUSP CE entry leads to a continuation area which
begins by entry CE or ST, then these entries were skipped without
interpretation. In case of CE this would lead to premature end of
processing the SUSP entries of the file. In case of ST this could
cause following non-SUSP bytes to be interpreted as SUSP entries.
Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>
Tested-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
An SL entry consists of the entry info and the component area.
The entry info should take up 5 bytes instead of sizeof(*entry).
The area after the first 5 bytes is the component area. It is
incorrect to use the sizeof(*entry) to check the entry boundary.
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In the code, the for loop advanced the entry pointer to the next entry before
checking if the next entry is within the system use area boundary. Another
issue in the code was that there is no check for the size of system use area.
For a corrupted system, the size of system use area can be less than the size
of minimum SUSP entry size (4 bytes). These can cause buffer overrun. The fixes
added the checks to ensure the read is valid and within the boundary.
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
There is no check for the end of block when reading
directory extents. It resulted in read_node() always
read from the same offset in the while loop, thus
caused infinite loop. The fix added a check for the
end of the block and ensure the read is within directory
boundary.
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Oops. You're allowed to have up to n = NAT_JOURNAL_ENTRIES entries
_inclusive_, because the loop below uses i < n, not i <= n. D'oh.
Fixes: 4bd9877f62 (fs/f2fs: Do not read past the end of nat journal entries)
Reported-by: программист нект <programmer11180@programist.ru>
Tested-by: программист нект <programmer11180@programist.ru>
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The l1_entries and l2_entries were not freed at the end of file read.
Fixes: 5825b3794 (BFS implementation based on the specification)
Signed-off-by: t.feng <fengtao40@huawei.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The hashtable is not freed if GRUB_AFFS_FILETYPE_HARDLINK and
grub_disk_read() failed. If grub_affs_create_node() returns non-zero
the hashtable should be freed too.
By the way, the hashtable argument is unused in grub_affs_create_node().
So, we can remove the argument and free it in grub_affs_iterate_dir().
It allocates the memory and it should be responsible for releasing it.
This is why commit ebf32bc4e9 (fs/affs: Fix resource leaks) missed
this memory leak.
Fixes: ebf32bc4e9 (fs/affs: Fix resource leaks)
Signed-off-by: t.feng <fengtao40@huawei.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Validate the length of Allocation Extent Descriptor in grub_udf_read_block(),
based on the details in UDF spec. v2.01 section 2.3.11.
Fixes: CID 314037
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
The dnode_get_path() traverses dnode structures to locate the dnode leaf
of a given path. When the leaf is a symlink to another path, it restarts
the traversal either from root or from a different path. In such cases,
dn_new must be re-initialized
Passes "make check".
Fixes: CID 86750
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Ross Philipson <ross.philipson@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Recently, ext4 added the large_dir feature, which adds support for
a 3 level htree directory support.
The GRUB supports existing file systems with htree directories by
ignoring their existence, and since the index nodes for the hash tree
look like deleted directory entries (by design), the GRUB can simply do
a brute force O(n) linear search of directories. The same is true for
3 level deep htrees indicated by large_dir feature flag.
Hence, it is safe for the GRUB to ignore the large_dir incompat feature.
Fixes: https://savannah.gnu.org/bugs/?61606
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Coverity reports that dnode_end_t argument of fill_fs_info() is too
large to pass-by-value. Therefore, replace the argument with a pointer.
Fixes: CID 73631
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
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>
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>
Alec Brown