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
