On Thu, May 30, 2024 at 06:57:36AM +0800, lei lu wrote: > This adds sanity checks for xfs_dir2_data_unused and xfs_dir2_data_entry > to make sure don't stray beyond valid memory region. It just checks start > offset < end without checking end offset < end. Well, it does do this checking, but it assumes that the dup/dep headers fit in the buffer because of entry size and alignment constraints. > So if last entry is > xfs_dir2_data_unused, and is located at the end of ag. Not sure what this means. > We can change > dup->length to dup->length-1 and leave 1 byte of space. Ah, so not a real-world issue in any way. Regardless, this is the corruption we are failing to catch. All the structures in the directory name area should be 8 byte aligned, and we should be catching dup->length % XFS_DIR2_DATA_ALIGN != 0 and reporting that as corruption. This also means that the smallest valid length for dup->length is xfs_dir2_data_entsize(mp, 1), except if it is the last entry in the block (i.e. at end - offset == XFS_DIR2_DATA_ALIGN), in which case it may be XFS_DIR2_DATA_ALIGN bytes in length. IOWs, we're failing to check for both the alignment and the size constraints on the dup->length field, and that's the problem we need to fix to address the out of bounds read error being reported. Can you please rework the patch to catch the corruption you induced at the exact point we are processing the corrupt object, rather than try to catch an overrun that might happen several iterations after the corrupt object itself was processed? > In the next > traversal, this space will be considered as dup or dep. We may encounter > an out-of-bound read when accessing the fixed members. Verifiers are supposed to validate each object in the structure is within specification, not be coded simply to prevent out of bounds accesses. i.e. if the next traversal trips over an out of bounds access, then one of the previous iobject verifications failed to detect an out of bounds value that it should not have missed. > Signed-off-by: lei lu <llfamsec@xxxxxxxxx> > --- > fs/xfs/libxfs/xfs_dir2_data.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c > index dbcf58979a59..08c18e0c1baa 100644 > --- a/fs/xfs/libxfs/xfs_dir2_data.c > +++ b/fs/xfs/libxfs/xfs_dir2_data.c > @@ -178,6 +178,9 @@ __xfs_dir3_data_check( > struct xfs_dir2_data_unused *dup = bp->b_addr + offset; > struct xfs_dir2_data_entry *dep = bp->b_addr + offset; > > + if (offset + sizeof(*dup) > end) > + return __this_address; > + > /* > * If it's unused, look for the space in the bestfree table. > * If we find it, account for that, else make sure it > @@ -210,6 +213,10 @@ __xfs_dir3_data_check( > lastfree = 1; > continue; > } > + > + if (offset + sizeof(*dep) > end) > + return __this_address; That doesn't look correct - dep has a variable sized array and tail packed information in it that sizeof(*dep) doesn't take into account. The actual size of the dep structure we need to consider here is going to be a minimum sized entry - xfs_dir2_data_entsize(mp, 1) - as anything smaller than this size is definitely invalid and we shouldn't attempt to decode any of it. -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
