Re: [PATCH] fs/ntfs3: validate the EA value length in ntfs_read_ea()

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

On 8/3/26 06:34, Yuejie Shi wrote:

> ntfs_read_ea() checks the $EA list for consistency, but it only
> recomputes the packed size of an entry when the entry's own size field is
> zero:
>
>          for (off = 0; off < size; off += ea_size) {
>                  const struct EA_FULL *ef = Add2Ptr(ea_p, off);
>                  u32 bytes = size - off;
>                  ...
>                  if (ef->size) {
>                          ea_size = le32_to_cpu(ef->size);
>                          if (ea_size > bytes)
>                                  goto out1;
>                          continue;              /* <- elength never checked */
>                  }
>                  ...
>                  ea_size = ALIGN(struct_size(ef, name,
>                                              1 + ef->name_len +
>                                                      le16_to_cpu(ef->elength)),
>                                  4);
>                  if (ea_size > bytes)
>                          goto out1;
>          }
>
> ef->size is non-zero for every entry except the last one, and it is what
> ntfs3 itself writes (ntfs_set_ea() stores exactly that aligned packed
> size), so in practice elength is not validated at all.  find_ea() then
> uses unpacked_ea_size(), which also prefers ea->size when it is set, so
> it agrees and hands the entry back.
>
> ntfs_get_ea() copies elength bytes with no further bound of its own:
>
>          len = le16_to_cpu(ea->elength);
>          ...
>          if (len > size) {              /* size is the *caller's* buffer */
>                  err = -ERANGE;
>                  ...
>          }
>          memcpy(buffer, ea->name + ea->name_len + 1, len);
>
> so a caller that supplies a large enough buffer receives up to 64 KiB of
> whatever follows the small $EA allocation -- straight to userspace via
> getxattr(2).
>
> Compute the packed size unconditionally and require it to fit in the
> entry.  Hoist the "bytes < offsetof(struct EA_FULL, name)" test above the
> ef->size branch as well: name_len and elength were being read there
> without it, which is itself a small over-read of the kmalloc'd buffer
> when fewer than 8 bytes are left.  The old "bytes < sizeof(ef->size)"
> test is subsumed by it.
>
>    # mkntfs image; create /mnt/ntfs/poc with user.ntfs6 set, then rewrite
>    # that EA_FULL's elength from 0xf to 0xff00 on the raw device
>    mount -t ntfs3 -o ro /dev/vda /mnt/ntfs
>    getxattr("/mnt/ntfs/poc", "user.ntfs6", buf, 128 * 1024);
>
>    BUG: KASAN: slab-out-of-bounds in ntfs_get_ea+0x294/0x3e0
>    Read of size 65280 at addr ffff0000cca53acf by task xattr_probe/142
>     __asan_memcpy+0x3c/0xa0
>     ntfs_get_ea+0x294/0x3e0
>     ntfs_getxattr+0x284/0x318
>     __vfs_getxattr+0x104/0x160
>     vfs_getxattr+0x1b4/0x1e0
>     do_getxattr+0xcc/0x230
>     path_getxattrat+0x184/0x270
>     __arm64_sys_getxattr+0x64/0x80
>    Allocated by task 142:
>     __kmalloc_noprof+0x294/0x668
>     ntfs_read_ea+0x1e4/0x420
>     ntfs_get_ea+0x2c8/0x3e0
>    The buggy address belongs to the object at ffff0000cca53a80
>     which belongs to the cache kmalloc-96 of size 96
>
> The object being over-read is the one ntfs_read_ea() just allocated, and
> it is 96 bytes.
>
> Entries written by ntfs3 and by Windows set size to the aligned packed
> size, so the new test does not reject anything a working filesystem
> produces.
>
> Fixes: be71b5cba2e6 ("fs/ntfs3: Add attrib operations")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yuejie Shi <syjcnss@gmail.com>
> ---
>   fs/ntfs3/xattr.c | 27 ++++++++++++++++-----------
>   1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> index 04814dd29375..e67a1100d0cd 100644
> --- a/fs/ntfs3/xattr.c
> +++ b/fs/ntfs3/xattr.c
> @@ -146,26 +146,31 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct
>          for (off = 0; off < size; off += ea_size) {
>                  const struct EA_FULL *ef = Add2Ptr(ea_p, off);
>                  u32 bytes = size - off;
> +               u32 packed_size;
>
> -               /* Check if we can use field ea->size. */
> -               if (bytes < sizeof(ef->size))
> +               /* Check if we can use fields ef->name_len and ef->elength. */
> +               if (bytes < offsetof(struct EA_FULL, name))
>                          goto out1;
>
> +               /*
> +                * The name and the value have to fit in the entry whether or
> +                * not ef->size is set, because ntfs_get_ea() copies elength
> +                * bytes out of this buffer and checks them only against the
> +                * caller supplied buffer size.
> +                */
> +               packed_size = ALIGN(struct_size(ef, name,
> +                                               1 + ef->name_len +
> +                                                       le16_to_cpu(ef->elength)),
> +                                   4);
> +
>                  if (ef->size) {
>                          ea_size = le32_to_cpu(ef->size);
> -                       if (ea_size > bytes)
> +                       if (ea_size > bytes || packed_size > ea_size)
>                                  goto out1;
>                          continue;
>                  }
>
> -               /* Check if we can use fields ef->name_len and ef->elength. */
> -               if (bytes < offsetof(struct EA_FULL, name))
> -                       goto out1;
> -
> -               ea_size = ALIGN(struct_size(ef, name,
> -                                           1 + ef->name_len +
> -                                                   le16_to_cpu(ef->elength)),
> -                               4);
> +               ea_size = packed_size;
>                  if (ea_size > bytes)
>                          goto out1;
>          }
> --
> 2.51.0

Hello,

The patch was applied, thank you.

Regards,
Konstantin