Re: [PATCH 3/4] ntfs: validate index entries on reading

[CharSyam]

Hi, Hyunchul.

 One small thing — the guard accepts an $INDEX_ROOT shorter than a full struct
index_root, so &ir->index can point at a 16-byte index_header that extends
past the end of the resident value:

        u32 value_length = le32_to_cpu(a->data.resident.value_length);

        if (value_length < offsetof(struct index_root, index)) {
                ntfs_error(vol->sb, "$INDEX_ROOT in inode %llu is too small.",
                           (unsigned long long)inum);
                return -EIO;
        }

 It would be safer to compare against sizeof(struct index_root) instead, since
that guarantees the full index_header lies within the value before
ntfs_index_header_inconsistent() dereferences it:

        if (value_length < sizeof(struct index_root)) {

Thanks.
DaeMyung.

2026년 5월 22일 (금) 오전 9:49, Hyunchul Lee <hyc.lee@xxxxxxxxx>님이 작성:
>
> Validate index entries immediately after reading an index root or index
> block from disk. This eliminates repeated checks in lookup and readdir,
> and reduce the risk of missing checks in those paths.
>
> Signed-off-by: Hyunchul Lee <hyc.lee@xxxxxxxxx>
> ---
>  fs/ntfs/dir.c   | 28 ++---------------
>  fs/ntfs/index.c | 96 +++++++++++++++++++++++++++++++--------------------------
>  fs/ntfs/index.h |  8 +++--
>  fs/ntfs/inode.c |  8 +++--
>  4 files changed, 66 insertions(+), 74 deletions(-)
>
> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
> index 6745a0e6e3e7..4b6bd5f30c65 100644
> --- a/fs/ntfs/dir.c
> +++ b/fs/ntfs/dir.c
> @@ -135,10 +135,6 @@ u64 ntfs_lookup_inode_by_name(struct ntfs_inode *dir_ni, const __le16 *uname,
>                 /* Key length should not be zero if it is not last entry. */
>                 if (!ie->key_length)
>                         goto dir_err_out;
> -               /* Check the consistency of an index entry */
> -               if (ntfs_index_entry_inconsistent(NULL, vol, ie, COLLATION_FILE_NAME,
> -                               dir_ni->mft_no))
> -                       goto dir_err_out;
>                 /*
>                  * We perform a case sensitive comparison and if that matches
>                  * we are done and return the mft reference of the inode (i.e.
> @@ -351,7 +347,8 @@ u64 ntfs_lookup_inode_by_name(struct ntfs_inode *dir_ni, const __le16 *uname,
>         }
>         err = ntfs_index_block_inconsistent(vol, ia,
>                                             dir_ni->itype.index.block_size,
> -                                           vcn, dir_ni->mft_no);
> +                                           vcn, COLLATION_FILE_NAME,
> +                                           dir_ni->mft_no);
>         if (err)
>                 goto unm_err_out;
>         index_end = (u8 *)&ia->index + le32_to_cpu(ia->index.index_length);
> @@ -364,15 +361,6 @@ u64 ntfs_lookup_inode_by_name(struct ntfs_inode *dir_ni, const __le16 *uname,
>          * reach the last entry.
>          */
>         for (;; ie = (struct index_entry *)((u8 *)ie + le16_to_cpu(ie->length))) {
> -               /* Bounds checks. */
> -               if ((u8 *)ie < (u8 *)ia ||
> -                   (u8 *)ie + sizeof(struct index_entry_header) > index_end ||
> -                   (u8 *)ie + sizeof(struct index_entry_header) + le16_to_cpu(ie->key_length) >
> -                               index_end || (u8 *)ie + le16_to_cpu(ie->length) > index_end) {
> -                       ntfs_error(sb, "Index entry out of bounds in directory inode 0x%llx.",
> -                                       dir_ni->mft_no);
> -                       goto unm_err_out;
> -               }
>                 /*
>                  * The last entry cannot contain a name. It can however contain
>                  * a pointer to a child node in the B+tree so we just break out.
> @@ -382,10 +370,6 @@ u64 ntfs_lookup_inode_by_name(struct ntfs_inode *dir_ni, const __le16 *uname,
>                 /* Key length should not be zero if it is not last entry. */
>                 if (!ie->key_length)
>                         goto unm_err_out;
> -               /* Check the consistency of an index entry */
> -               if (ntfs_index_entry_inconsistent(NULL, vol, ie, COLLATION_FILE_NAME,
> -                               dir_ni->mft_no))
> -                       goto unm_err_out;
>                 /*
>                  * We perform a case sensitive comparison and if that matches
>                  * we are done and return the mft reference of the inode (i.e.
> @@ -868,6 +852,7 @@ static int ntfs_readdir(struct file *file, struct dir_context *actor)
>                 ictx->vcn_size_bits = vol->cluster_size_bits;
>         else
>                 ictx->vcn_size_bits = NTFS_BLOCK_SIZE_BITS;
> +       ictx->cr = ir->collation_rule;
>
>         /* The first index entry. */
>         next = (struct index_entry *)((u8 *)&ir->index +
> @@ -905,13 +890,6 @@ static int ntfs_readdir(struct file *file, struct dir_context *actor)
>                 if (!next)
>                         break;
>  nextdir:
> -               /* Check the consistency of an index entry */
> -               if (ntfs_index_entry_inconsistent(ictx, vol, next, COLLATION_FILE_NAME,
> -                                       ndir->mft_no)) {
> -                       err = -EIO;
> -                       goto out;
> -               }
> -
>                 if (ie_pos < actor->pos) {
>                         ie_pos += le16_to_cpu(next->length);
>                         continue;
> diff --git a/fs/ntfs/index.c b/fs/ntfs/index.c
> index 91fcaa79f8ac..456e195ca5c9 100644
> --- a/fs/ntfs/index.c
> +++ b/fs/ntfs/index.c
> @@ -28,41 +28,10 @@
>   * length must have been checked beforehand to not overflow from the
>   * index record.
>   */
> -int ntfs_index_entry_inconsistent(struct ntfs_index_context *icx,
> -               struct ntfs_volume *vol, const struct index_entry *ie,
> -               __le32 collation_rule, u64 inum)
> +static int ntfs_index_entry_inconsistent(const struct ntfs_volume *vol,
> +                                        const struct index_entry *ie,
> +                                        __le32 collation_rule, u64 inum)
>  {
> -       if (icx) {
> -               struct index_header *ih;
> -               u8 *ie_start, *ie_end;
> -
> -               if (icx->is_in_root)
> -                       ih = &icx->ir->index;
> -               else
> -                       ih = &icx->ib->index;
> -
> -               if ((le32_to_cpu(ih->index_length) > le32_to_cpu(ih->allocated_size)) ||
> -                               (le32_to_cpu(ih->index_length) > icx->block_size)) {
> -                       ntfs_error(vol->sb, "%s Index entry(0x%p)'s length is too big.",
> -                                       icx->is_in_root ? "Index root" : "Index block",
> -                                       (u8 *)icx->entry);
> -                       return -EINVAL;
> -               }
> -
> -               ie_start = (u8 *)ih + le32_to_cpu(ih->entries_offset);
> -               ie_end = (u8 *)ih + le32_to_cpu(ih->index_length);
> -
> -               if (ie_start > (u8 *)ie ||
> -                   ie_end <= (u8 *)ie + le16_to_cpu(ie->length) ||
> -                   le16_to_cpu(ie->length) > le32_to_cpu(ih->allocated_size) ||
> -                   le16_to_cpu(ie->length) > icx->block_size) {
> -                       ntfs_error(vol->sb, "Index entry(0x%p) is out of range from %s",
> -                                       (u8 *)icx->entry,
> -                                       icx->is_in_root ? "index root" : "index block");
> -                       return -EIO;
> -               }
> -       }
> -
>         if (ie->key_length &&
>             ((le16_to_cpu(ie->key_length) + offsetof(struct index_entry, key)) >
>              le16_to_cpu(ie->length))) {
> @@ -350,6 +319,44 @@ static int ntfs_index_header_inconsistent(struct ntfs_volume *vol,
>         return 0;
>  }
>
> +int ntfs_index_entries_inconsistent(const struct ntfs_volume *vol,
> +                                   const struct index_header *ih,
> +                                   __le32 collation_rule, u64 inum)
> +{
> +       struct index_entry *ie;
> +       u8 *index_end = (u8 *)ih + le32_to_cpu(ih->index_length);
> +
> +       for (ie = ntfs_ie_get_first((struct index_header *)ih);
> +             ; ie = ntfs_ie_get_next(ie)) {
> +               if ((u8 *)ie + sizeof(struct index_entry_header) > index_end ||
> +                   (u8 *)ie + le16_to_cpu(ie->length) > index_end) {
> +                       ntfs_error(vol->sb,
> +                                  "Index entry out of bounds in inode %llu.",
> +                                  (unsigned long long)inum);
> +                       return -EIO;
> +               }
> +
> +               if (le16_to_cpu(ie->length) < sizeof(struct index_entry_header)) {
> +                       ntfs_error(vol->sb,
> +                                  "Index etnry too small in inode %llu.",
> +                                  inum);
> +                       return -EIO;
> +               }
> +
> +               if (ntfs_ie_end(ie))
> +                       break;
> +
> +               if (!ie->key_length)
> +                       return -EIO;
> +
> +               if (ntfs_index_entry_inconsistent(vol, ie,
> +                                                 collation_rule, inum))
> +                       return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
>  /*
>   *  Find the last entry in the index block
>   */
> @@ -501,7 +508,8 @@ static struct index_entry *ntfs_ie_dup_novcn(struct index_entry *ie)
>   */
>  int ntfs_index_block_inconsistent(struct ntfs_volume *vol,
>                                   const struct index_block *ib,
> -                                 u32 block_size, s64 vcn, u64 inum)
> +                                 u32 block_size, s64 vcn, __le32 cr,
> +                                 u64 inum)
>  {
>         u32 ib_size = (unsigned int)le32_to_cpu(ib->index.allocated_size) +
>                 offsetof(struct index_block, index);
> @@ -512,7 +520,7 @@ int ntfs_index_block_inconsistent(struct ntfs_volume *vol,
>         if (!ntfs_is_indx_record(ib->magic)) {
>                 ntfs_error(sb, "Corrupt index block signature: vcn %lld inode %llu\n",
>                            vcn, (unsigned long long)inum);
> -               return -1;
> +               return -EIO;
>         }
>
>         if (le64_to_cpu(ib->index_block_vcn) != vcn) {
> @@ -520,22 +528,23 @@ int ntfs_index_block_inconsistent(struct ntfs_volume *vol,
>                         "Corrupt index block: s64 (%lld) is different from expected s64 (%lld) in inode %llu\n",
>                         (long long)le64_to_cpu(ib->index_block_vcn),
>                         vcn, inum);
> -               return -1;
> +               return -EIO;
>         }
>
>         if (ib_size != block_size) {
>                 ntfs_error(sb,
>                            "Corrupt index block : s64 (%lld) of inode %llu has a size (%u) differing from the index specified size (%u)\n",
>                            vcn, inum, ib_size, block_size);
> -               return -1;
> +               return -EIO;
>         }
>
>         if (ntfs_index_header_inconsistent(vol, &ib->index,
>                                            block_size -
>                                            offsetof(struct index_block, index),
>                                            inum))
> -               return -1;
> -
> +               return -EIO;
> +       if (ntfs_index_entries_inconsistent(vol, &ib->index, cr, inum))
> +               return -EIO;
>         return 0;
>  }
>
> @@ -720,15 +729,14 @@ static int ntfs_ib_read(struct ntfs_index_context *icx, s64 vcn, struct index_bl
>                 else
>                         ntfs_error(icx->idx_ni->vol->sb,
>                                 "Failed to read full index block at %lld\n", pos);
> -               return -1;
> +               return -EIO;
>         }
>
>         post_read_mst_fixup((struct ntfs_record *)((u8 *)dst), icx->block_size);
>         if (ntfs_index_block_inconsistent(icx->idx_ni->vol, dst,
> -                                         icx->block_size, vcn,
> +                                         icx->block_size, vcn, icx->cr,
>                                           icx->idx_ni->mft_no))
> -               return -1;
> -
> +               return -EIO;
>         return 0;
>  }
>
> diff --git a/fs/ntfs/index.h b/fs/ntfs/index.h
> index cad78568d8b3..9a03f53bba47 100644
> --- a/fs/ntfs/index.h
> +++ b/fs/ntfs/index.h
> @@ -94,9 +94,11 @@ int ntfs_index_root_inconsistent(struct ntfs_volume *vol,
>                                  const struct index_root *ir, u64 inum);
>  int ntfs_index_block_inconsistent(struct ntfs_volume *vol,
>                                   const struct index_block *ib,
> -                                 u32 block_size, s64 vcn, u64 inum);
> -int ntfs_index_entry_inconsistent(struct ntfs_index_context *icx, struct ntfs_volume *vol,
> -               const struct index_entry *ie, __le32 collation_rule, u64 inum);
> +                                 u32 block_size, s64 vcn,
> +                                 __le32 cr, u64 inum);
> +int ntfs_index_entries_inconsistent(const struct ntfs_volume *vol,
> +                                   const struct index_header *ih,
> +                                   __le32 collation_rule, u64 inum);
>  struct ntfs_index_context *ntfs_index_ctx_get(struct ntfs_inode *ni, __le16 *name,
>                 u32 name_len);
>  void ntfs_index_ctx_put(struct ntfs_index_context *ictx);
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index 63ee7acff4fc..9717fb5b4709 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -939,7 +939,9 @@ static int ntfs_read_locked_inode(struct inode *vi)
>                 }
>                 ir = (struct index_root *)((u8 *)a +
>                                 le16_to_cpu(a->data.resident.value_offset));
> -               if (ntfs_index_root_inconsistent(ni->vol, a, ir, ni->mft_no)) {
> +               if (ntfs_index_root_inconsistent(ni->vol, a, ir, ni->mft_no) ||
> +                   ntfs_index_entries_inconsistent(ni->vol, &ir->index,
> +                                                   ir->collation_rule, ni->mft_no)) {
>                         ntfs_error(vi->i_sb, "Directory index is corrupt.");
>                         goto unm_err_out;
>                 }
> @@ -1529,7 +1531,9 @@ static int ntfs_read_locked_index_inode(struct inode *base_vi, struct inode *vi)
>         }
>
>         ir = (struct index_root *)((u8 *)a + le16_to_cpu(a->data.resident.value_offset));
> -       if (ntfs_index_root_inconsistent(vol, a, ir, ni->mft_no)) {
> +       if (ntfs_index_root_inconsistent(vol, a, ir, ni->mft_no) ||
> +           ntfs_index_entries_inconsistent(vol, &ir->index,
> +                                           ir->collation_rule, ni->mft_no)) {
>                 ntfs_error(vi->i_sb, "Index is corrupt.");
>                 goto unm_err_out;
>         }
>
> --
> 2.43.0
>
>