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

[Hyunchul Lee]

2026년 5월 22일 (금) 오후 9:59, CharSyam <charsyam@xxxxxxxxx>님이 작성:
>
> 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)) {
>

The original intent was for ntfs_index_header_inconsistent() to validate that
space is available before referencing index_header. However the current
accesses index_header fields before the validation. I will update the function
to check the remaining space first as originally intended.

> 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
> >
> >



--
Thanks,
Hyunchul