[PATCH] fs/ntfs3: fix UAF in al_destroy() on stream inode eviction

[PATCH] fs/ntfs3: fix UAF in al_destroy() on stream inode eviction

[Henry Martin]

al_destroy() unconditionally dereferences ni->base and frees the shared
base attribute list:

    void al_destroy(struct ntfs_inode *ni)
    {
        ni = ni->base;
        run_close(&ni->attr_list.run);
        kvfree(ni->attr_list.le);
        ni->attr_list.le = NULL;
        ni->attr_list.size = 0;
        ni->attr_list.dirty = false;
    }

ni_clear() (fs/ntfs3/frecord.c) calls al_destroy() for every inode
being evicted, including ADS stream inodes whose ->base still points
at the base inode: the base's attr_list.le is freed while the base
inode is still alive (iput(&ni->base->vfs_inode) happens only at the
end of ni_clear()), racing the base's ni_lock-held attr_list consumers
(attr_set_size, al_add_le, ntfs_load_attr_list).

Stream inodes have no attr_list of their own; freeing the base's list
on their eviction also leaves the base inode with a destroyed list
(size=0, le=NULL) even without a race.

Only destroy the attribute list for inodes that own it: a stream
inode's ->base points at another inode, while on a base inode it is
self-referencing (fs/ntfs3/inode.c), so the ownership check is
ni->base == ni.

This issue was discovered by Tencent CodeBuddy Security.

Fixes: be71b5cba2e6 ("fs/ntfs3: Add attrib operations")
Cc: stable@vger.kernel.org
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
 fs/ntfs3/attrlist.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/ntfs3/attrlist.c b/fs/ntfs3/attrlist.c
index 8710560cf2944..6aced486a45c7 100644
--- a/fs/ntfs3/attrlist.c
+++ b/fs/ntfs3/attrlist.c
@@ -29,7 +29,17 @@ static inline bool al_is_valid_le(const struct ntfs_inode *ni,
 
 void al_destroy(struct ntfs_inode *ni)
 {
-	ni = ni->base;
+	struct ntfs_inode *base = ni->base;
+
+	/* Stream inodes share the base's attribute list; freeing it here
+	 * would race the base's ni_lock-held consumers and leave the base
+	 * with a destroyed list while it is still alive.  Only the owner
+	 * frees.
+	 */
+	if (base != ni)
+		return;
+
+	ni = base;
 	run_close(&ni->attr_list.run);
 	kvfree(ni->attr_list.le);
 	ni->attr_list.le = NULL;
-- 
2.43.7

[PATCH] fs/ntfs3: fix NULL deref in ni_std5() on stream inodes

[Henry Martin]

ni_std5() looks up the standard information attribute in the inode's
own MFT record:

    attr = mi_find_attr(ni, &ni->mi, NULL, ATTR_STD, NULL, 0, NULL);

without redirecting to the base inode.  For an ADS stream inode,
ni->mi.mrec is never initialized (ntfs_init_ads_node() in
fs/ntfs3/inode.c leaves mi.mrec NULL), so mi_find_attr() ->
mi_enum_attr() dereferences rec == NULL:

    BUG: unable to handle kernel NULL pointer dereference
    RIP: mi_enum_attr+0x18/0x180

The SYSTEM_NTFS_SECURITY xattr path calls ni_std5() without a guard
for stream inodes (fs/ntfs3/xattr.c): ntfs_set_acl()/ntfs_xattr_set()
reach it via ntfs_insert_security -> ni_lock(ni) -> ni_std5(ni) on the
stream inode, and the stream inode inherits the base's i_mode/uid/gid,
so the may_write_xattr() ownership check passes for the file owner.
The default stream-name parsing (colons) is enabled, so an
unprivileged local user can open("file:stream"), set the security
descriptor xattr on it and panic the kernel.

Redirect stream inodes to their base record in ni_std5().

This issue was discovered by Tencent CodeBuddy Security.

Fixes: be71b5cba2e6 ("fs/ntfs3: Add attrib operations")
Cc: stable@vger.kernel.org
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
 fs/ntfs3/frecord.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index bead01a953f3f..dce6400195aee 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -89,6 +89,13 @@ struct ATTR_STD_INFO5 *ni_std5(struct ntfs_inode *ni)
 {
 	const struct ATTRIB *attr;
 
+	/* Stream inodes have no MFT record of their own (mi.mrec is
+	 * NULL); the standard information attribute lives in the base
+	 * record.
+	 */
+	if (ni->base && ni->base != ni)
+		ni = ni->base;
+
 	attr = mi_find_attr(ni, &ni->mi, NULL, ATTR_STD, NULL, 0, NULL);
 
 	return attr ? resident_data_ex(attr, sizeof(struct ATTR_STD_INFO5)) :
-- 
2.43.7