| Message ID | 20240529225736.21028-1-llfamsec@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: don't walk off the end of a directory data block | expand |
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@gmail.com> > --- > 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.
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@fromorbit.com> 于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@gmail.com> > > --- > > 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@fromorbit.com
On Thu, May 30, 2024 at 11:10:57AM +0800, lei lu wrote: > 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. Sure, but go back and read what I said. Detect the actual object corruption, not the downstream symptom. IOWs, the verifier should be detecting the exact corruption you induced. Catching all the object corruptions prevents a buffer overrun. We abort processing before we move beyond the end of the buffer. IOWs, we need to: 1. verify dup->length is a multiple of XFS_DIR2_DATA_ALIGN; and 2. verify that if the last object in the buffer is less than xfs_dir2_data_entsize(mp, 1) bytes in size it must be a dup entry of exactly XFS_DIR2_DATA_ALIGN bytes in length. If either of these checks fail, then the block is corrupt. #1 will catch your induced corruption and fail immediately. #2 will catch the runt entry in the structure without derefencing past the end of the structure. Can you now see how properly validating that the objects within the structure will prevent buffer overruns from occurring without needing generic buffer overrun checks? -Dave.
Got it. I will send a V2 patch to do a proper validation. On Mon, Jun 3, 2024 at 1:58 PM Dave Chinner <david@fromorbit.com> wrote: > > On Thu, May 30, 2024 at 11:10:57AM +0800, lei lu wrote: > > 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. > > Sure, but go back and read what I said. > > Detect the actual object corruption, not the downstream symptom. > > IOWs, the verifier should be detecting the exact corruption you > induced. > > Catching all the object corruptions prevents a buffer overrun. > We abort processing before we move beyond the end of the buffer. > > IOWs, we need to: > > 1. verify dup->length is a multiple of XFS_DIR2_DATA_ALIGN; and > 2. verify that if the last object in the buffer is less than > xfs_dir2_data_entsize(mp, 1) bytes in size it must be a dup > entry of exactly XFS_DIR2_DATA_ALIGN bytes in length. > > If either of these checks fail, then the block is corrupt. > #1 will catch your induced corruption and fail immediately. > #2 will catch the runt entry in the structure without derefencing > past the end of the structure. > > Can you now see how properly validating that the objects within the > structure will prevent buffer overruns from occurring without > needing generic buffer overrun checks? > > -Dave. > -- > Dave Chinner > david@fromorbit.com
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; + /* * It's a real entry. Validate the fields. * If this is a block directory then make sure it's
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. So if last entry is xfs_dir2_data_unused, and is located at the end of ag. We can change dup->length to dup->length-1 and leave 1 byte of space. 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. Signed-off-by: lei lu <llfamsec@gmail.com> --- fs/xfs/libxfs/xfs_dir2_data.c | 7 +++++++ 1 file changed, 7 insertions(+)