Re: [PATCH] fs/ntfs3: prevent attribute-list pointer underflow

[Konstantin Komarov <almaz.alexandrovich@paragon-software.com>]

> attr_set_size_ex() and attr_collapse_range() use the on-disk size of a
> removed ATTR_LIST_ENTRY to step a pointer backward. A crafted entry whose
> size exceeds its offset from the attribute-list buffer start underflows the
> pointer, and the next entry dereference reads outside the allocation.
>
> BUG: KASAN: slab-out-of-bounds in attr_set_size_ex
>    attr_set_size_ex fs/ntfs3/attrib.c:847
>    ntfs_file_release fs/ntfs3/file.c:1427
>    __fput fs/file_table.c:512
>    __x64_sys_close fs/open.c:1496
>
> Before each backward step, reject an entry whose on-disk size exceeds its
> offset from the attribute-list buffer start. This prevents pointer
> underflow in both the shrink and collapse-range paths while preserving
> their existing fail-stop error handling.
>
> Commit be71b5cba2e6 ("fs/ntfs3: Add attrib operations") introduced the
> vulnerable implementation. Commit 12dad495eaab ("fs/ntfs3: Add Kconfig,
> Makefile and doc") added the local build files. Commit 6e5be40d32fb
> ("fs/ntfs3: Add NTFS3 in fs/Kconfig and fs/Makefile") wired them into
> the top-level build, making NTFS3 triggerable in standard kernels.
>
> Fixes: 6e5be40d32fb ("fs/ntfs3: Add NTFS3 in fs/Kconfig and fs/Makefile")
> Reported-by: AutonomousCodeSecurity@microsoft.com
> Reported-by: Xiang Mei (Microsoft) <xmei5@asu.edu>
> Reported-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
> Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
> ---
>   fs/ntfs3/attrib.c | 19 ++++++++++++++-----
>   1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c
> index c621a4c582f9..562bfcf3561f 100644
> --- a/fs/ntfs3/attrib.c
> +++ b/fs/ntfs3/attrib.c
> @@ -779,12 +779,15 @@ int attr_set_size_ex(struct ntfs_inode *ni, enum ATTR_TYPE type,
>                          u16 le_sz = le16_to_cpu(le->size);
>
>                          /*
> -                        * NOTE: List entries for one attribute are always
> -                        * the same size. We deal with last entry (vcn==0)
> -                        * and it is not first in entries array
> -                        * (list entry for std attribute always first).
> -                        * So it is safe to step back.
> +                        * List entries for one attribute are expected to have
> +                        * the same size. Validate the on-disk size before using
> +                        * it to step back.
>                           */
> +                       if (le_sz > PtrOffset(ni->attr_list.le, le)) {
> +                               err = -EINVAL;
> +                               goto bad_inode;
> +                       }
> +
>                          mi_remove_attr(NULL, mi, attr);
>
>                          if (!al_remove_le(ni, le)) {
> @@ -2216,6 +2219,12 @@ int attr_collapse_range(struct ntfs_inode *ni, u64 vbo, u64 bytes)
>                                  }
>                                  continue;
>                          }
> +
> +                       if (le_sz > PtrOffset(ni->attr_list.le, le)) {
> +                               err = -EINVAL;
> +                               goto out;
> +                       }
> +
>                          le = (struct ATTR_LIST_ENTRY *)((u8 *)le - le_sz);
>                  }
>
> --
> 2.52.0

Hello,

Sorry for the delay.
The patch was applied, thank you.

Regards,
Konstantin