Re: [PATCH 09/14] fs/ntfs3: Check fields while reading

From: Lee Jones
Date: Tue Jun 27 2023 - 09:50:09 EST

Next message: David Howells: "[PATCH net-next v3] libceph: Partially revert changes to support MSG_SPLICE_PAGES"
Previous message: Breno Leitao: "[PATCH v4] io_uring: Add io_uring command support for sockets"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Konstantin,

On Mon, 19 Jun 2023, Lee Jones wrote:
> On Fri, 28 Oct 2022, Konstantin Komarov wrote:
>
> > Added new functions index_hdr_check and index_buf_check.
> > Now we check all stuff for correctness while reading from disk.
> > Also fixed bug with stale nfs data.
> >
> > Reported-by: van fantasy <g1042620637@xxxxxxxxx>
> > Signed-off-by: Konstantin Komarov <almaz.alexandrovich@xxxxxxxxxxxxxxxxxxxx>
> > ---
> > fs/ntfs3/index.c | 84 ++++++++++++++++++++++++++++++----
> > fs/ntfs3/inode.c | 18 ++++----
> > fs/ntfs3/ntfs_fs.h | 4 +-
> > fs/ntfs3/run.c | 7 ++-
> > fs/ntfs3/xattr.c | 109 +++++++++++++++++++++++++++++----------------
> > 5 files changed, 164 insertions(+), 58 deletions(-)
>
> It's not clear to me what route this patch took into Mainline [0]. My
> guess it was authored by and then merged by the maintainer after no one
> raised any review comments.
>
> It does appear that this particular patch was identified as the fix for
> a published CVE however [1], and with no Fixes: tag I'm concerned that
> the Stable AUTOSEL bot may miss this. With that in mind, would you mind
> submitting to the relevant Stable branches please? [2] contains all of
> the relevant information if you're not 100% certain of the process.
>
> Thank you.
>
> [0] 0e8235d28f3a0 fs/ntfs3: Check fields while reading
> [1] https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-48502
> [2] Documentation/process/stable-kernel-rules.rst

Can you confirm receipt of this mail please?

Would you be kind enough to submit your patch to Stable please?

> > diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
> > index 35369ae5c438..51ab75954640 100644
> > --- a/fs/ntfs3/index.c
> > +++ b/fs/ntfs3/index.c
> > @@ -605,11 +605,58 @@ static const struct NTFS_DE *hdr_insert_head(struct INDEX_HDR *hdr,
> > return e;
> > }
> > +/*
> > + * index_hdr_check
> > + *
> > + * return true if INDEX_HDR is valid
> > + */
> > +static bool index_hdr_check(const struct INDEX_HDR *hdr, u32 bytes)
> > +{
> > + u32 end = le32_to_cpu(hdr->used);
> > + u32 tot = le32_to_cpu(hdr->total);
> > + u32 off = le32_to_cpu(hdr->de_off);
> > +
> > + if (!IS_ALIGNED(off, 8) || tot > bytes || end > tot ||
> > + off + sizeof(struct NTFS_DE) > end) {
> > + /* incorrect index buffer. */
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +/*
> > + * index_buf_check
> > + *
> > + * return true if INDEX_BUFFER seems is valid
> > + */
> > +static bool index_buf_check(const struct INDEX_BUFFER *ib, u32 bytes,
> > + const CLST *vbn)
> > +{
> > + const struct NTFS_RECORD_HEADER *rhdr = &ib->rhdr;
> > + u16 fo = le16_to_cpu(rhdr->fix_off);
> > + u16 fn = le16_to_cpu(rhdr->fix_num);
> > +
> > + if (bytes <= offsetof(struct INDEX_BUFFER, ihdr) ||
> > + rhdr->sign != NTFS_INDX_SIGNATURE ||
> > + fo < sizeof(struct INDEX_BUFFER)
> > + /* Check index buffer vbn. */
> > + || (vbn && *vbn != le64_to_cpu(ib->vbn)) || (fo % sizeof(short)) ||
> > + fo + fn * sizeof(short) >= bytes ||
> > + fn != ((bytes >> SECTOR_SHIFT) + 1)) {
> > + /* incorrect index buffer. */
> > + return false;
> > + }
> > +
> > + return index_hdr_check(&ib->ihdr,
> > + bytes - offsetof(struct INDEX_BUFFER, ihdr));
> > +}
> > +
> > void fnd_clear(struct ntfs_fnd *fnd)
> > {
> > int i;
> > - for (i = 0; i < fnd->level; i++) {
> > + for (i = fnd->level - 1; i >= 0; i--) {
> > struct indx_node *n = fnd->nodes[i];
> > if (!n)
> > @@ -819,9 +866,16 @@ int indx_init(struct ntfs_index *indx, struct ntfs_sb_info *sbi,
> > u32 t32;
> > const struct INDEX_ROOT *root = resident_data(attr);
> > + t32 = le32_to_cpu(attr->res.data_size);
> > + if (t32 <= offsetof(struct INDEX_ROOT, ihdr) ||
> > + !index_hdr_check(&root->ihdr,
> > + t32 - offsetof(struct INDEX_ROOT, ihdr))) {
> > + goto out;
> > + }
> > +
> > /* Check root fields. */
> > if (!root->index_block_clst)
> > - return -EINVAL;
> > + goto out;
> > indx->type = type;
> > indx->idx2vbn_bits = __ffs(root->index_block_clst);
> > @@ -833,19 +887,19 @@ int indx_init(struct ntfs_index *indx, struct ntfs_sb_info *sbi,
> > if (t32 < sbi->cluster_size) {
> > /* Index record is smaller than a cluster, use 512 blocks. */
> > if (t32 != root->index_block_clst * SECTOR_SIZE)
> > - return -EINVAL;
> > + goto out;
> > /* Check alignment to a cluster. */
> > if ((sbi->cluster_size >> SECTOR_SHIFT) &
> > (root->index_block_clst - 1)) {
> > - return -EINVAL;
> > + goto out;
> > }
> > indx->vbn2vbo_bits = SECTOR_SHIFT;
> > } else {
> > /* Index record must be a multiple of cluster size. */
> > if (t32 != root->index_block_clst << sbi->cluster_bits)
> > - return -EINVAL;
> > + goto out;
> > indx->vbn2vbo_bits = sbi->cluster_bits;
> > }
> > @@ -853,7 +907,14 @@ int indx_init(struct ntfs_index *indx, struct ntfs_sb_info *sbi,
> > init_rwsem(&indx->run_lock);
> > indx->cmp = get_cmp_func(root);
> > - return indx->cmp ? 0 : -EINVAL;
> > + if (!indx->cmp)
> > + goto out;
> > +
> > + return 0;
> > +
> > +out:
> > + ntfs_set_state(sbi, NTFS_DIRTY_DIRTY);
> > + return -EINVAL;
> > }
> > static struct indx_node *indx_new(struct ntfs_index *indx,
> > @@ -1011,6 +1072,13 @@ int indx_read(struct ntfs_index *indx, struct ntfs_inode *ni, CLST vbn,
> > goto out;
> > ok:
> > + if (!index_buf_check(ib, bytes, &vbn)) {
> > + ntfs_inode_err(&ni->vfs_inode, "directory corrupted");
> > + ntfs_set_state(ni->mi.sbi, NTFS_DIRTY_ERROR);
> > + err = -EINVAL;
> > + goto out;
> > + }
> > +
> > if (err == -E_NTFS_FIXUP) {
> > ntfs_write_bh(ni->mi.sbi, &ib->rhdr, &in->nb, 0);
> > err = 0;
> > @@ -1601,9 +1669,9 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> > if (err) {
> > /* Restore root. */
> > - if (mi_resize_attr(mi, attr, -ds_root))
> > + if (mi_resize_attr(mi, attr, -ds_root)) {
> > memcpy(attr, a_root, asize);
> > - else {
> > + } else {
> > /* Bug? */
> > ntfs_set_state(sbi, NTFS_DIRTY_ERROR);
> > }
> > diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
> > index 78ec3e6bbf67..719cf6fbb5ed 100644
> > --- a/fs/ntfs3/inode.c
> > +++ b/fs/ntfs3/inode.c
> > @@ -81,7 +81,7 @@ static struct inode *ntfs_read_mft(struct inode *inode,
> > le16_to_cpu(ref->seq), le16_to_cpu(rec->seq));
> > goto out;
> > } else if (!is_rec_inuse(rec)) {
> > - err = -EINVAL;
> > + err = -ESTALE;
> > ntfs_err(sb, "Inode r=%x is not in use!", (u32)ino);
> > goto out;
> > }
> > @@ -92,8 +92,10 @@ static struct inode *ntfs_read_mft(struct inode *inode,
> > goto out;
> > }
> > - if (!is_rec_base(rec))
> > - goto Ok;
> > + if (!is_rec_base(rec)) {
> > + err = -EINVAL;
> > + goto out;
> > + }
> > /* Record should contain $I30 root. */
> > is_dir = rec->flags & RECORD_FLAG_DIR;
> > @@ -465,7 +467,6 @@ static struct inode *ntfs_read_mft(struct inode *inode,
> > inode->i_flags |= S_NOSEC;
> > }
> > -Ok:
> > if (ino == MFT_REC_MFT && !sb->s_root)
> > sbi->mft.ni = NULL;
> > @@ -519,6 +520,9 @@ struct inode *ntfs_iget5(struct super_block *sb, const struct MFT_REF *ref,
> > _ntfs_bad_inode(inode);
> > }
> > + if (IS_ERR(inode) && name)
> > + ntfs_set_state(sb->s_fs_info, NTFS_DIRTY_ERROR);
> > +
> > return inode;
> > }
> > @@ -1652,10 +1656,8 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
> > ntfs_remove_reparse(sbi, IO_REPARSE_TAG_SYMLINK, &new_de->ref);
> > out5:
> > - if (S_ISDIR(mode) || run_is_empty(&ni->file.run))
> > - goto out4;
> > -
> > - run_deallocate(sbi, &ni->file.run, false);
> > + if (!S_ISDIR(mode))
> > + run_deallocate(sbi, &ni->file.run, false);
> > out4:
> > clear_rec_inuse(rec);
> > diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
> > index d73d1c837ba7..c9b8a6f1ba0b 100644
> > --- a/fs/ntfs3/ntfs_fs.h
> > +++ b/fs/ntfs3/ntfs_fs.h
> > @@ -795,12 +795,12 @@ int run_pack(const struct runs_tree *run, CLST svcn, CLST len, u8 *run_buf,
> > u32 run_buf_size, CLST *packed_vcns);
> > int run_unpack(struct runs_tree *run, struct ntfs_sb_info *sbi, CLST ino,
> > CLST svcn, CLST evcn, CLST vcn, const u8 *run_buf,
> > - u32 run_buf_size);
> > + int run_buf_size);
> > #ifdef NTFS3_CHECK_FREE_CLST
> > int run_unpack_ex(struct runs_tree *run, struct ntfs_sb_info *sbi, CLST ino,
> > CLST svcn, CLST evcn, CLST vcn, const u8 *run_buf,
> > - u32 run_buf_size);
> > + int run_buf_size);
> > #else
> > #define run_unpack_ex run_unpack
> > #endif
> > diff --git a/fs/ntfs3/run.c b/fs/ntfs3/run.c
> > index aaaa0d3d35a2..12d8682f33b5 100644
> > --- a/fs/ntfs3/run.c
> > +++ b/fs/ntfs3/run.c
> > @@ -919,12 +919,15 @@ int run_pack(const struct runs_tree *run, CLST svcn, CLST len, u8 *run_buf,
> > */
> > int run_unpack(struct runs_tree *run, struct ntfs_sb_info *sbi, CLST ino,
> > CLST svcn, CLST evcn, CLST vcn, const u8 *run_buf,
> > - u32 run_buf_size)
> > + int run_buf_size)
> > {
> > u64 prev_lcn, vcn64, lcn, next_vcn;
> > const u8 *run_last, *run_0;
> > bool is_mft = ino == MFT_REC_MFT;
> > + if (run_buf_size < 0)
> > + return -EINVAL;
> > +
> > /* Check for empty. */
> > if (evcn + 1 == svcn)
> > return 0;
> > @@ -1046,7 +1049,7 @@ int run_unpack(struct runs_tree *run, struct ntfs_sb_info *sbi, CLST ino,
> > */
> > int run_unpack_ex(struct runs_tree *run, struct ntfs_sb_info *sbi, CLST ino,
> > CLST svcn, CLST evcn, CLST vcn, const u8 *run_buf,
> > - u32 run_buf_size)
> > + int run_buf_size)
> > {
> > int ret, err;
> > CLST next_vcn, lcn, len;
> > diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> > index aeee5fb12092..385c50831a8d 100644
> > --- a/fs/ntfs3/xattr.c
> > +++ b/fs/ntfs3/xattr.c
> > @@ -42,28 +42,26 @@ static inline size_t packed_ea_size(const struct EA_FULL *ea)
> > * Assume there is at least one xattr in the list.
> > */
> > static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
> > - const char *name, u8 name_len, u32 *off)
> > + const char *name, u8 name_len, u32 *off, u32 *ea_sz)
> > {
> > - *off = 0;
> > + u32 ea_size;
> > - if (!ea_all || !bytes)
> > + *off = 0;
> > + if (!ea_all)
> > return false;
> > - for (;;) {
> > + for (; *off < bytes; *off += ea_size) {
> > const struct EA_FULL *ea = Add2Ptr(ea_all, *off);
> > - u32 next_off = *off + unpacked_ea_size(ea);
> > -
> > - if (next_off > bytes)
> > - return false;
> > -
> > + ea_size = unpacked_ea_size(ea);
> > if (ea->name_len == name_len &&
> > - !memcmp(ea->name, name, name_len))
> > + !memcmp(ea->name, name, name_len)) {
> > + if (ea_sz)
> > + *ea_sz = ea_size;
> > return true;
> > -
> > - *off = next_off;
> > - if (next_off >= bytes)
> > - return false;
> > + }
> > }
> > +
> > + return false;
> > }
> > /*
> > @@ -74,12 +72,12 @@ static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
> > static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea,
> > size_t add_bytes, const struct EA_INFO **info)
> > {
> > - int err;
> > + int err = -EINVAL;
> > struct ntfs_sb_info *sbi = ni->mi.sbi;
> > struct ATTR_LIST_ENTRY *le = NULL;
> > struct ATTRIB *attr_info, *attr_ea;
> > void *ea_p;
> > - u32 size;
> > + u32 size, off, ea_size;
> > static_assert(le32_to_cpu(ATTR_EA_INFO) < le32_to_cpu(ATTR_EA));
> > @@ -96,24 +94,31 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea,
> > *info = resident_data_ex(attr_info, sizeof(struct EA_INFO));
> > if (!*info)
> > - return -EINVAL;
> > + goto out;
> > /* Check Ea limit. */
> > size = le32_to_cpu((*info)->size);
> > - if (size > sbi->ea_max_size)
> > - return -EFBIG;
> > + if (size > sbi->ea_max_size) {
> > + err = -EFBIG;
> > + goto out;
> > + }
> > +
> > + if (attr_size(attr_ea) > sbi->ea_max_size) {
> > + err = -EFBIG;
> > + goto out;
> > + }
> > - if (attr_size(attr_ea) > sbi->ea_max_size)
> > - return -EFBIG;
> > + if (!size) {
> > + /* EA info persists, but xattr is empty. Looks like EA problem. */
> > + goto out;
> > + }
> > /* Allocate memory for packed Ea. */
> > ea_p = kmalloc(size_add(size, add_bytes), GFP_NOFS);
> > if (!ea_p)
> > return -ENOMEM;
> > - if (!size) {
> > - /* EA info persists, but xattr is empty. Looks like EA problem. */
> > - } else if (attr_ea->non_res) {
> > + if (attr_ea->non_res) {
> > struct runs_tree run;
> > run_init(&run);
> > @@ -124,24 +129,52 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea,
> > run_close(&run);
> > if (err)
> > - goto out;
> > + goto out1;
> > } else {
> > void *p = resident_data_ex(attr_ea, size);
> > - if (!p) {
> > - err = -EINVAL;
> > - goto out;
> > - }
> > + if (!p)
> > + goto out1;
> > memcpy(ea_p, p, size);
> > }
> > memset(Add2Ptr(ea_p, size), 0, add_bytes);
> > +
> > + /* Check all attributes for consistency. */
> > + for (off = 0; off < size; off += ea_size) {
> > + const struct EA_FULL *ef = Add2Ptr(ea_p, off);
> > + u32 bytes = size - off;
> > +
> > + /* Check if we can use field ea->size. */
> > + if (bytes < sizeof(ef->size))
> > + goto out1;
> > +
> > + if (ef->size) {
> > + ea_size = le32_to_cpu(ef->size);
> > + if (ea_size > bytes)
> > + 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);
> > + if (ea_size > bytes)
> > + goto out1;
> > + }
> > +
> > *ea = ea_p;
> > return 0;
> > -out:
> > +out1:
> > kfree(ea_p);
> > - *ea = NULL;
> > +out:
> > + ntfs_set_state(sbi, NTFS_DIRTY_DIRTY);
> > return err;
> > }
> > @@ -163,6 +196,7 @@ static ssize_t ntfs_list_ea(struct ntfs_inode *ni, char *buffer,
> > const struct EA_FULL *ea;
> > u32 off, size;
> > int err;
> > + int ea_size;
> > size_t ret;
> > err = ntfs_read_ea(ni, &ea_all, 0, &info);
> > @@ -175,8 +209,9 @@ static ssize_t ntfs_list_ea(struct ntfs_inode *ni, char *buffer,
> > size = le32_to_cpu(info->size);
> > /* Enumerate all xattrs. */
> > - for (ret = 0, off = 0; off < size; off += unpacked_ea_size(ea)) {
> > + for (ret = 0, off = 0; off < size; off += ea_size) {
> > ea = Add2Ptr(ea_all, off);
> > + ea_size = unpacked_ea_size(ea);
> > if (buffer) {
> > if (ret + ea->name_len + 1 > bytes_per_buffer) {
> > @@ -227,7 +262,8 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len,
> > goto out;
> > /* Enumerate all xattrs. */
> > - if (!find_ea(ea_all, le32_to_cpu(info->size), name, name_len, &off)) {
> > + if (!find_ea(ea_all, le32_to_cpu(info->size), name, name_len, &off,
> > + NULL)) {
> > err = -ENODATA;
> > goto out;
> > }
> > @@ -269,7 +305,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> > struct EA_FULL *new_ea;
> > struct EA_FULL *ea_all = NULL;
> > size_t add, new_pack;
> > - u32 off, size;
> > + u32 off, size, ea_sz;
> > __le16 size_pack;
> > struct ATTRIB *attr;
> > struct ATTR_LIST_ENTRY *le;
> > @@ -304,9 +340,8 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> > size_pack = ea_info.size_pack;
> > }
> > - if (info && find_ea(ea_all, size, name, name_len, &off)) {
> > + if (info && find_ea(ea_all, size, name, name_len, &off, &ea_sz)) {
> > struct EA_FULL *ea;
> > - size_t ea_sz;
> > if (flags & XATTR_CREATE) {
> > err = -EEXIST;
> > @@ -329,8 +364,6 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> > if (ea->flags & FILE_NEED_EA)
> > le16_add_cpu(&ea_info.count, -1);
> > - ea_sz = unpacked_ea_size(ea);
> > -
> > le16_add_cpu(&ea_info.size_pack, 0 - packed_ea_size(ea));
> > memmove(ea, Add2Ptr(ea, ea_sz), size - off - ea_sz);
> > --
> > 2.37.0
> >
> >
>
> --
> Lee Jones [李琼斯]

--
Lee Jones [李琼斯]

Next message: David Howells: "[PATCH net-next v3] libceph: Partially revert changes to support MSG_SPLICE_PAGES"
Previous message: Breno Leitao: "[PATCH v4] io_uring: Add io_uring command support for sockets"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]