linux-kernel - Re: [PATCH] drm/i915: Fix refcount leak and possible NULL pointer dereference.

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: <201312260045.rBQ0j2TD059809@www262.sakura.ne.jp>
Date:	Thu, 26 Dec 2013 09:45:02 +0900
From:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:	chris@...is-wilson.co.uk
Cc:	ben@...dawsk.net, daniel.vetter@...ll.ch,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/i915: Fix refcount leak and possible NULL pointer dereference.

Hello.

Chris Wilson wrote:
> > Since get_pid_task() grabs a reference on the task_struct, we have to drop the
> > refcount after reading that task\'s comm name. Also, directly reading like
> > get_pid_task()->comm can trigger an oops when get_pid_task() returned NULL.
> 
> The second issue is moot as file itself cannot exist if the task_struct
> is NULL, and the task_struct cannot be destroyed until we finish the
> function. The simpler fix would appear to be s/get_pid_task/pid_task/

I didn\'t catch. Please see below sample module.

pid_task() will return NULL if the target task has already died.
The target task could be killed (e.g. by the OOM killer) at any time, and
whether the target task\'s task_struct is not yet destroyed is irrelevant
for pid_task().

get_pid_task(file->pid, PIDTYPE_PID)->comm by chance didn\'t trigger
oops because offsetof(struct task_struct, comm) < PAGE_SIZE is true.

---------- sample start ----------
#include <linux/module.h>
#include <linux/delay.h>
#include <linux/kthread.h>

static int test_thread(void *unused)
{
        return 0;
}

static int __init test_init(void)
{
        struct task_struct *task = kthread_create(test_thread, NULL, \"test\");
        if (!IS_ERR(task)) {
                struct pid *pid;
                struct task_struct *t;
                get_task_struct(task);
                pid = get_pid(task->pids[PIDTYPE_PID].pid);
                rcu_read_lock();
                t = pid_task(pid, PIDTYPE_PID);
                rcu_read_unlock();
                printk(\"pid_task(%p)=%p\\n\", pid, t);
                wake_up_process(task);
                ssleep(1);
                rcu_read_lock();
                t = pid_task(pid, PIDTYPE_PID);
                rcu_read_unlock();
                printk(\"pid_task(%p)=%p\\n\", pid, t);
                put_task_struct(task);
                put_pid(pid);
        }
        return -EINVAL;
}

module_init(test_init);
MODULE_LICENSE(\"GPL\");
---------- sample end ----------

---------- output start ----------
pid_task(ffff8800474bff80)=ffff8800435b0e10
pid_task(ffff8800474bff80)=          (null)
---------- output end ----------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/