Thanks for your time. I just add check for the fixed members because I see after the patch code there is some checks for dup and dep. "offset + be16_to_cpu(dup->length) > end" for dup and "offset + xfs_dir2_data_entsize(mp, dep->namelen) > end" for dep. “xfs_dir2_data_entsize(mp, dep->namelen)” ensures the alignment of the dep. Dave Chinner <david@xxxxxxxxxxxxx> 于2024年5月30日周四 10:38写道： > > 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
