Re: [PATCH v4] fs/xfs: Fix XFS directory extent parsing

[Marta Lewandowska <mlewando@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4282 bytes --]

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<nuxi@vault24.org>
> Reviewed-by: Daniel Kiper<daniel.kiper@oracle.com>
>
> 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/ZS9H5exHL1g3aK37@feynman.vault24.org/T/#m57b1df94e2df65aaff444a89eec3b543184b6f07
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #1.2: Type: text/html, Size: 5757 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel