lkml.org 
[lkml]   [2017]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH] exit: move exit_task_namespaces() after exit_task_work()
Date
Cong Wang <xiyou.wangcong@gmail.com> writes:

> On Thu, Dec 14, 2017 at 1:08 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Thu, Dec 14, 2017 at 12:17:57PM -0800, Cong Wang wrote:
>>> syzbot reported we have a use-after-free when mqueue_evict_inode()
>>> is called on __cleanup_mnt() path, where the ipc ns is already
>>> freed by the previous exit_task_namespaces(). We can just move
>>> it after after exit_task_work() to avoid this use-after-free.
>>
>> What's to prevent somebody else holding a reference to the same
>> inode past the exit(2)? IOW, I don't believe that this is fixing
>> anything - in the best case, your patch papers over a specific
>> reproducer.
>
> You are right, I missed mq_clear_sbinfo().

yes, the patch 9c583773d036336176e9e50441890659bc4eeae8 introduced an
issue since it doesn't reset s_fs_info when ns->mq_mnt == NULL.

The following patch solves the issue for me. I store the superblock in
"struct ipc_namespace" since we cannot assume "mq_mnt" exists.

Does the patch look fine? I'll clean it up and submit it separately if
this approach is correct.

Regards,
Giuseppe


diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
index 554e31494f69..66d4a5740a60 100644
--- a/include/linux/ipc_namespace.h
+++ b/include/linux/ipc_namespace.h
@@ -53,6 +53,7 @@ struct ipc_namespace {

/* The kern_mount of the mqueuefs sb. We take a ref on it */
struct vfsmount *mq_mnt;
+ struct super_block *mq_sb;

/* # queues in this ns, protected by mq_lock */
unsigned int mq_queues_count;
diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index f78d6687a61b..16347cf1de2d 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -341,6 +341,7 @@ static int mqueue_fill_super(struct super_block *sb, void *data, int silent)
sb->s_root = d_make_root(inode);
if (!sb->s_root)
return -ENOMEM;
+ ns->mq_sb = sb;
return 0;
}

@@ -1554,6 +1555,7 @@ int mq_init_ns(struct ipc_namespace *ns, bool mount)
ns->mq_msg_max = DFLT_MSGMAX;
ns->mq_msgsize_max = DFLT_MSGSIZEMAX;
ns->mq_msg_default = DFLT_MSG;
+ ns->mq_sb = NULL;
ns->mq_msgsize_default = DFLT_MSGSIZE;

if (!mount)
@@ -1573,8 +1575,8 @@ int mq_init_ns(struct ipc_namespace *ns, bool mount)

void mq_clear_sbinfo(struct ipc_namespace *ns)
{
- if (ns->mq_mnt)
- ns->mq_mnt->mnt_sb->s_fs_info = NULL;
+ if (ns->mq_sb)
+ ns->mq_sb->s_fs_info = NULL;
}

void mq_put_mnt(struct ipc_namespace *ns)
\
 
 \ /
  Last update: 2017-12-16 23:05    [W:0.048 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site