Re: [PATCH v2] ntfs: serialize attribute-list replacement with lookups

[Hyunchul Lee]

Hi Cen,

Hi Cen,

I reviewed and noticed a few concurrency issues.

* ntfs_attr_lookup() releases the read lock before returning, which leaves
the ctx->al_entry unprotected during the lifetime of the search context.

* In some functions such as ntfs_attrlist_entry_add(), the memcpy()
reads base_ni->attr_list outside of the writ lock. If another thread
frees it concurrently, this leads to a UAF.

To resolve these issues, How about coupling the attr_list_lock directly
with the search context?

* Extend ntfs_attr_get_search_ctx with a lock mode (NONE, READ, WRITE)
to automatically acquire the lock unpon context creation and release
it in ntfs_attr_put_search_ctx().

* To prevent deadlocks, the following lock ordering is needed:
base_ni->mrec_lock => base_ni->attr_list_lock => ni->runlist.lock.
And to prevent nesting attr_list_lock under runlist.lock, The NONE
mode bypasses context-owned locking and is required for paths
that must control ordering manually (e.g, ntfs_attr_vcn_to_lcn_nlock()).

2026년 6월 27일 (토) 오후 1:28, Cen Zhang <zzzccc427@xxxxxxxxx>님이 작성:
>
> ntfs_external_attr_find() walks base_ni->attr_list with raw
> attr_list_entry cursors stored in ctx->al_entry and local variables.  Other
> metadata paths can replace base_ni->attr_list and free the old buffer while
> a lookup still carries one of those cursors.
>
> The buggy scenario involves two paths, with each column showing the order
> within that path:
>
>   attribute lookup path:              attribute-list mutation path:
>   1. enters ntfs_attr_lookup()        1. enters an attr-list add/remove
>   2. reads base_ni->attr_list         2. builds a replacement buffer
>   3. keeps ctx->al_entry/al_entry     3. publishes base_ni->attr_list
>      pointing into that buffer        4. frees the previous buffer
>   4. continues the attribute search
>   5. dereferences the saved cursor
>
> Validation reproduced this kernel report:
> BUG: KASAN: slab-use-after-free in ntfs_attr_lookup+0x24b2/0x2bf0
>
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x66/0xa0
>  print_report+0xce/0x630
>  ? ntfs_attr_lookup+0x24b2/0x2bf0
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? __virt_addr_valid+0x20d/0x410
>  ? ntfs_attr_lookup+0x24b2/0x2bf0
>  kasan_report+0xe0/0x110
>  ? ntfs_attr_lookup+0x24b2/0x2bf0
>  ntfs_attr_lookup+0x24b2/0x2bf0
>  ? ntfs_attr_lookup+0x9/0x2bf0
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? 0xffffffffc00000da
>  ? __pfx_ntfs_attr_lookup+0x10/0x10
>  ntfs_attr_map_whole_runlist+0x488/0x8b0
>  ? __pfx_ntfs_attr_map_whole_runlist+0x10/0x10
>  ? __pfx_do_raw_spin_trylock+0x10/0x10
>  ntfs_attr_vcn_to_rl+0x13a/0x1d0
>  ? filemap_fault+0xf03/0x2080
>  ntfs_read_iomap_begin_non_resident+0x142/0x920
>  ? __pfx_ntfs_read_iomap_begin_non_resident+0x10/0x10
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? __pfx_ntfs_read_iomap_begin+0x10/0x10
>  iomap_iter+0x6de/0x11e0
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? prepare_alloc_pages+0x1c4/0x4d0
>  iomap_readahead+0x239/0x910
>  ? folios_put_refs+0x1ed/0x4f0
>  ? __pfx_iomap_readahead+0x10/0x10
>  ? __pfx_folios_put_refs+0x10/0x10
>  ? lock_release+0x1e0/0x280
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? xas_store+0x871/0x1650
>  ? xas_find_conflict+0x64b/0xb90
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? lock_release+0x1e0/0x280
>  ntfs_readahead+0x193/0x1d0
>  ? __pfx_ntfs_readahead+0x10/0x10
>  ? __filemap_add_folio+0x869/0xa60
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? lock_acquire+0x2b8/0x2f0
>  ? __pfx___filemap_add_folio+0x10/0x10
>  read_pages+0x18f/0x890
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? folio_alloc_noprof+0x69/0xb0
>  ? __pfx_read_pages+0x10/0x10
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  page_cache_ra_unbounded+0x389/0x6c0
>  do_page_cache_ra+0x109/0x170
>  filemap_fault+0xf03/0x2080
>  ? __pfx_filemap_fault+0x10/0x10
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? lock_acquire+0x2b8/0x2f0
>  ? lockdep_init_map_type+0x5c/0x230
>  __do_fault+0xf0/0x260
>  __handle_mm_fault+0xdcf/0x1ca0
>  ? __pfx___handle_mm_fault+0x10/0x10
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? lock_release+0x1e0/0x280
>  handle_mm_fault+0x19c/0x470
>  do_user_addr_fault+0x23b/0x9c0
>  exc_page_fault+0x5c/0xc0
>  asm_exc_page_fault+0x26/0x30
>
> Allocated by task 511:
>  kasan_save_stack+0x33/0x60
>  kasan_save_track+0x14/0x30
>  __kasan_kmalloc+0xaa/0xb0
>  __kvmalloc_node_noprof+0x353/0x920
>  ntfs_attrlist_entry_rm+0x28d/0x510
>  ntfs_attr_record_rm+0x4dd/0xda0
>  ntfs_delete+0x50a/0xf90
>  ntfs_unlink+0x212/0x4d0
>  vfs_unlink+0x259/0xb00
>  filename_unlinkat+0x2db/0x5f0
>  __x64_sys_unlink+0x46/0x70
>  do_syscall_64+0x115/0x6a0
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
>
> Freed by task 511:
>  kasan_save_stack+0x33/0x60
>  kasan_save_track+0x14/0x30
>  kasan_save_free_info+0x3b/0x60
>  __kasan_slab_free+0x5f/0x80
>  kfree+0x307/0x580
>  ntfs_attrlist_entry_add+0xc1b/0x1110
>  ntfs_resident_attr_record_add+0x6a8/0xa10
>  ntfs_attr_add+0x682/0xcd0
>  __ntfs_link+0x815/0xde0
>  ntfs_link+0x223/0x3d0
>  vfs_link+0x465/0xcf0
>  filename_linkat+0x316/0x540
>  __x64_sys_link+0x81/0xb0
>  do_syscall_64+0x115/0x6a0
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
>
> Add a per-ntfs_inode attr_list_lock.  ntfs_attr_lookup() takes the read
> side around the NInoAttrList() decision and the selected local or
> external lookup, so any attr-list cursor derived during the lookup is
> protected until the lookup returns.  Writers take the write side only
> while publishing, rolling back, detaching, or clearing the in-memory
> attr_list pointer, size, and state.  The on-disk ntfs_attrlist_update()
> work stays outside the write side.
>
> Fixes: 495e90fa3348 ("ntfs: update attrib operations")
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Cen Zhang <zzzccc427@xxxxxxxxx>
> ---
> v2:
> - Replace the read-iomap mrec_lock wrapper with a per-inode attr_list rwsem,
>   following Hyunchul Lee's review.
> - Protect attr-list creation, replacement, rollback, detach, and clear
>   transitions, while keeping ntfs_attrlist_update() outside the write side.
> - Update the Fixes tag to the attribute operations commit.
>
>  fs/ntfs/attrib.c   | 21 +++++++++++++++++----
>  fs/ntfs/attrlist.c | 10 +++++++++-
>  fs/ntfs/inode.c    | 10 ++++++++++
>  fs/ntfs/inode.h    |  4 ++++
>  4 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index dd8828098511..e630f63ed7ad 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -1565,6 +1565,7 @@ int ntfs_attr_lookup(const __le32 type, const __le16 *name,
>                 struct ntfs_attr_search_ctx *ctx)
>  {
>         struct ntfs_inode *base_ni;
> +       int err;
>
>         ntfs_debug("Entering.");
>         if (ctx->base_ntfs_ino)
> @@ -1572,11 +1573,18 @@ int ntfs_attr_lookup(const __le32 type, const __le16 *name,
>         else
>                 base_ni = ctx->ntfs_ino;
>         /* Sanity check, just for debugging really. */
> -       if (!base_ni || !NInoAttrList(base_ni) || type == AT_ATTRIBUTE_LIST)
> +       if (!base_ni)
>                 return ntfs_attr_find(type, name, name_len, ic, val, val_len,
>                                 ctx);
> -       return ntfs_external_attr_find(type, name, name_len, ic, lowest_vcn,
> -                       val, val_len, ctx);
> +
> +       down_read(&base_ni->attr_list_lock);
> +       if (!NInoAttrList(base_ni) || type == AT_ATTRIBUTE_LIST)
> +               err = ntfs_attr_find(type, name, name_len, ic, val, val_len, ctx);
> +       else
> +               err = ntfs_external_attr_find(type, name, name_len, ic,
> +                                             lowest_vcn, val, val_len, ctx);
> +       up_read(&base_ni->attr_list_lock);
> +       return err;
>  }
>
>  /**
> @@ -2622,6 +2630,7 @@ static int ntfs_non_resident_attr_record_add(struct ntfs_inode *ni, __le32 type,
>  int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
>  {
>         struct ntfs_inode *base_ni, *ni;
> +       u8 *old_al = NULL;
>         __le32 type;
>         int err;
>
> @@ -2659,10 +2668,14 @@ int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
>
>         /* Post $ATTRIBUTE_LIST delete setup. */
>         if (type == AT_ATTRIBUTE_LIST) {
> +               down_write(&base_ni->attr_list_lock);
>                 if (NInoAttrList(base_ni) && base_ni->attr_list)
> -                       kvfree(base_ni->attr_list);
> +                       old_al = base_ni->attr_list;
>                 base_ni->attr_list = NULL;
> +               base_ni->attr_list_size = 0;
>                 NInoClearAttrList(base_ni);
> +               up_write(&base_ni->attr_list_lock);
> +               kvfree(old_al);
>         }
>
>         /* Free MFT record, if it doesn't contain attributes. */
> diff --git a/fs/ntfs/attrlist.c b/fs/ntfs/attrlist.c
> index afb13038ba42..e91fc74eda31 100644
> --- a/fs/ntfs/attrlist.c
> +++ b/fs/ntfs/attrlist.c
> @@ -220,14 +220,18 @@ int ntfs_attrlist_entry_add(struct ntfs_inode *ni, struct attr_record *attr)
>                         entry_offset, ni->attr_list_size - entry_offset);
>
>         /* Set new runlist. */
> +       down_write(&ni->attr_list_lock);
>         old_al = ni->attr_list;
>         ni->attr_list = new_al;
>         ni->attr_list_size = ni->attr_list_size + entry_len;
> +       up_write(&ni->attr_list_lock);
>
>         err = ntfs_attrlist_update(ni);
>         if (err) {
> +               down_write(&ni->attr_list_lock);
>                 ni->attr_list = old_al;
>                 ni->attr_list_size -= entry_len;
> +               up_write(&ni->attr_list_lock);
>                 goto err_out;
>         }
>         kvfree(old_al);
> @@ -251,6 +255,7 @@ int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx)
>         int new_al_len;
>         struct ntfs_inode *base_ni;
>         struct attr_list_entry *ale;
> +       u8 *old_al;
>
>         if (!ctx || !ctx->ntfs_ino || !ctx->al_entry) {
>                 ntfs_debug("Invalid arguments.\n");
> @@ -285,9 +290,12 @@ int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx)
>                                 ale->length), new_al_len - ((u8 *)ale - base_ni->attr_list));
>
>         /* Set new runlist. */
> -       kvfree(base_ni->attr_list);
> +       down_write(&base_ni->attr_list_lock);
> +       old_al = base_ni->attr_list;
>         base_ni->attr_list = new_al;
>         base_ni->attr_list_size = new_al_len;
> +       up_write(&base_ni->attr_list_lock);
> +       kvfree(old_al);
>
>         return ntfs_attrlist_update(base_ni);
>  }
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index c2715521e562..25e957401892 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -475,6 +475,7 @@ void __ntfs_init_inode(struct super_block *sb, struct ntfs_inode *ni)
>         ni->folio = NULL;
>         ni->folio_ofs = 0;
>         ni->mrec = NULL;
> +       init_rwsem(&ni->attr_list_lock);
>         ni->attr_list_size = 0;
>         ni->attr_list = NULL;
>         ni->itype.index.block_size = 0;
> @@ -3112,9 +3113,11 @@ int ntfs_inode_add_attrlist(struct ntfs_inode *ni)
>         }
>
>         /* Set in-memory attribute list. */
> +       down_write(&ni->attr_list_lock);
>         ni->attr_list = al;
>         ni->attr_list_size = al_len;
>         NInoSetAttrList(ni);
> +       up_write(&ni->attr_list_lock);
>
>         attr_al_len = offsetof(struct attr_record, data.resident.reserved) + 1 +
>                 ((al_len + 7) & ~7);
> @@ -3147,8 +3150,11 @@ int ntfs_inode_add_attrlist(struct ntfs_inode *ni)
>
>  remove_attrlist_record:
>         /* Prevent ntfs_attr_recorm_rm from freeing attribute list. */
> +       down_write(&ni->attr_list_lock);
>         ni->attr_list = NULL;
> +       ni->attr_list_size = 0;
>         NInoClearAttrList(ni);
> +       up_write(&ni->attr_list_lock);
>         /* Remove $ATTRIBUTE_LIST record. */
>         ntfs_attr_reinit_search_ctx(ctx);
>         if (!ntfs_attr_lookup(AT_ATTRIBUTE_LIST, NULL, 0,
> @@ -3160,9 +3166,11 @@ int ntfs_inode_add_attrlist(struct ntfs_inode *ni)
>         }
>
>         /* Setup back in-memory runlist. */
> +       down_write(&ni->attr_list_lock);
>         ni->attr_list = al;
>         ni->attr_list_size = al_len;
>         NInoSetAttrList(ni);
> +       up_write(&ni->attr_list_lock);
>  rollback:
>         /*
>          * Scan attribute list for attributes that placed not in the base MFT
> @@ -3189,10 +3197,12 @@ int ntfs_inode_add_attrlist(struct ntfs_inode *ni)
>         }
>
>         /* Remove in-memory attribute list. */
> +       down_write(&ni->attr_list_lock);
>         ni->attr_list = NULL;
>         ni->attr_list_size = 0;
>         NInoClearAttrList(ni);
>         NInoClearAttrListDirty(ni);
> +       up_write(&ni->attr_list_lock);
>  put_err_out:
>         ntfs_attr_put_search_ctx(ctx);
>  err_out:
> diff --git a/fs/ntfs/inode.h b/fs/ntfs/inode.h
> index 9aacd5787ffe..0aaa6c90a7fb 100644
> --- a/fs/ntfs/inode.h
> +++ b/fs/ntfs/inode.h
> @@ -10,6 +10,8 @@
>  #ifndef _LINUX_NTFS_INODE_H
>  #define _LINUX_NTFS_INODE_H
>
> +#include <linux/rwsem.h>
> +
>  #include "debug.h"
>
>  enum ntfs_inode_mutex_lock_class {
> @@ -66,6 +68,7 @@ enum ntfs_inode_mutex_lock_class {
>   * Attribute list support (only for use by the attribute lookup
>   * functions). Setup during read_inode for all inodes with attribute
>   * lists. Only valid if NI_AttrList is set in state.
> + * @attr_list_lock: Serializes attribute list pointer updates with lookups.
>   * @attr_list_size: Length of attribute list value in bytes.
>   * @attr_list: Attribute list value itself.
>   *
> @@ -118,6 +121,7 @@ struct ntfs_inode {
>         int folio_ofs;
>         s64 mft_lcn[2];
>         unsigned int mft_lcn_count;
> +       struct rw_semaphore attr_list_lock;
>         u32 attr_list_size;
>         u8 *attr_list;
>         union {
> --
> 2.43.0



-- 
Thanks,
Hyunchul