netdev - Re: [PATCH] connector: fix unsafe usage of ->real

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <41085991551908921@iva1-4ad1e49a78b2.qloud-c.yandex.net>
Date:   Thu, 07 Mar 2019 00:48:41 +0300
From:   Evgeniy Polyakov <zbr@...emap.net>
To:     Li RongQing <lirongqing@...du.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH] connector: fix unsafe usage of ->real_parent

Hi

06.03.2019, 09:46, "Li RongQing" <lirongqing@...du.com>:
> proc_exit_connector() uses ->real_parent lockless. This is not
> safe that its parent can go away at any moment, so use RCU to
> protect it, and ensure that this task is not released.
>

> Fixes: b086ff87251b4a4 ("connector: add parent pid and tgid to coredump and exit events")
> Signed-off-by: Zhang Yu <zhangyu31@...du.com>
> Signed-off-by: Li RongQing <lirongqing@...du.com>

Looks good to me, thank you!
Acked-by: Evgeniy Polyakov <zbr@...emap.net>

> ---
>  drivers/connector/cn_proc.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
> index ed5e42461094..ad48fd52cb53 100644
> --- a/drivers/connector/cn_proc.c
> +++ b/drivers/connector/cn_proc.c
> @@ -250,6 +250,7 @@ void proc_coredump_connector(struct task_struct *task)
>  {
>          struct cn_msg *msg;
>          struct proc_event *ev;
> + struct task_struct *parent;
>          __u8 buffer[CN_PROC_MSG_SIZE] __aligned(8);
>
>          if (atomic_read(&proc_event_num_listeners) < 1)
> @@ -262,8 +263,14 @@ void proc_coredump_connector(struct task_struct *task)
>          ev->what = PROC_EVENT_COREDUMP;
>          ev->event_data.coredump.process_pid = task->pid;
>          ev->event_data.coredump.process_tgid = task->tgid;
> - ev->event_data.coredump.parent_pid = task->real_parent->pid;
> - ev->event_data.coredump.parent_tgid = task->real_parent->tgid;
> +
> + rcu_read_lock();
> + if (pid_alive(task)) {
> + parent = rcu_dereference(task->real_parent);
> + ev->event_data.coredump.parent_pid = parent->pid;
> + ev->event_data.coredump.parent_tgid = parent->tgid;
> + }
> + rcu_read_unlock();
>
>          memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
>          msg->ack = 0; /* not used */
> @@ -276,6 +283,7 @@ void proc_exit_connector(struct task_struct *task)
>  {
>          struct cn_msg *msg;
>          struct proc_event *ev;
> + struct task_struct *parent;
>          __u8 buffer[CN_PROC_MSG_SIZE] __aligned(8);
>
>          if (atomic_read(&proc_event_num_listeners) < 1)
> @@ -290,8 +298,14 @@ void proc_exit_connector(struct task_struct *task)
>          ev->event_data.exit.process_tgid = task->tgid;
>          ev->event_data.exit.exit_code = task->exit_code;
>          ev->event_data.exit.exit_signal = task->exit_signal;
> - ev->event_data.exit.parent_pid = task->real_parent->pid;
> - ev->event_data.exit.parent_tgid = task->real_parent->tgid;
> +
> + rcu_read_lock();
> + if (pid_alive(task)) {
> + parent = rcu_dereference(task->real_parent);
> + ev->event_data.exit.parent_pid = parent->pid;
> + ev->event_data.exit.parent_tgid = parent->tgid;
> + }
> + rcu_read_unlock();
>
>          memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
>          msg->ack = 0; /* not used */
> --
> 2.16.2