Hi,
I also applied both patches, and tested them on aarch64, ppc64le, and
on x64 UEFI and BIOS. Machines were all able to boot, and ournormal
gating tests passed.
-marta
On 10/18/23 18:23, Daniel Kiper wrote:
Adding Marta and Sebastian...
On Tue, Oct 17, 2023 at 11:03:47PM -0400, Jon DeVree wrote:
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 diretory 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 regressionn 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<[email protected]>
Reviewed-by: Daniel Kiper<[email protected]>
Jon, thank you for fixing this issue.
Marta, Sebastian, could you test this patch and patch [1] together?
Daniel
[1]https://lore.kernel.org/grub-devel/[email protected]/T/#m57b1df94e2df65aaff444a89eec3b543184b6f07
_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
