From: Tang Yizhou tangyizhou@huawei.com

ascend inclusion category: bugfix bugzilla: NA CVE: NA

------------------------------------------------- We encounter a problem of accessing NULL pointer of mm as below:

[25853.542104] Unable to handle kernel NULL pointer dereference at virtual address 000000000000004c [25853.542105] Mem abort info: [25853.542106] ESR = 0x96000006 [25853.542108] Exception class = DABT (current EL), IL = 32 bits [25853.542109] SET = 0, FnV = 0 [25853.542110] EA = 0, S1PTW = 0 [25853.542110] Data abort info: [25853.542111] ISV = 0, ISS = 0x00000006 [25853.542112] CM = 0, WnR = 0 [25853.542115] user pgtable: 4k pages, 48-bit VAs, pgdp = 0000000090ec4b55 [25853.542116] [000000000000004c] pgd=0000000d49b38003, pud=0000000d49be6003, pmd=0000000000000000 [25853.542121] Internal error: Oops: 96000006 [#1] SMP [25853.542123] Process dds_common_app (pid: 29004, stack limit = 0x000000001fcac39f) [25853.542127] CPU: 8 PID: 29004 Comm: dds_common_app Tainted: G O 4.19.95-1.h1.AOS2.0.aarch64 #1 [25853.542128] Hardware name: asic (DT) [25853.542129] pstate: 60400009 (nZCv daif +PAN -UAO) [25853.542136] pc : mmput+0x20/0x170 [25853.542141] lr : sp_make_share_u2k+0x1c0/0x2d8 [25853.542141] sp : ffff00000d263b80 [25853.542142] pmr_save: 000000e0 [25853.542143] x29: ffff00000d263b80 x28: 0000000000000007 [25853.542145] x27: 0000000000000007 x26: ffff00000d263cb0 [25853.542147] x25: ffff800d3d32d000 x24: ffff00000107f000 [25853.542148] x23: 0000000000007143 x22: 0000e80006000000 [25853.542150] x21: ffff00009b140000 x20: ffff0000095fa000 [25853.542151] x19: 0000000000000000 x18: ffff00000961b588 [25853.542153] x17: 0000000000000000 x16: 000000000000000e [25853.542154] x15: 0000000000000000 x14: ffff800d44d66418 [25853.542155] x13: 0000000000000000 x12: 0140000000000000 [25853.542157] x11: ffff0000db140000 x10: ffff0000096468f8 [25853.542158] x9 : ffff80089a517e80 x8 : 0000000000000008 [25853.542160] x7 : 00000000ffffffff x6 : 000000c4c5d56692 [25853.542161] x5 : 0000000000000000 x4 : 0000000000000020 [25853.542163] x3 : 0000000000000010 x2 : 000000000000004c [25853.542164] x1 : 0000000000000000 x0 : ffff00000831c718 [25853.542166] Call trace: [25853.542168] mmput+0x20/0x170 [25853.542169] sp_make_share_u2k+0x1c0/0x2d8 [25853.542180] mz_create+0x7c/0x328 [drv_buff_module] [25853.542185] buff_req_ioctl_mz_create+0xf8/0x338 [drv_buff_module] [25853.542189] buff_ioctl+0xc8/0x3f8 [drv_buff_module] [25853.542193] do_vfs_ioctl+0xc4/0x8c0 [25853.542194] ksys_ioctl+0x8c/0xa0 [25853.542196] __arm64_sys_ioctl+0x28/0x38 [25853.542200] el0_svc_common+0x8c/0x218 [25853.542202] el0_svc_handler+0x38/0x88 [25853.542205] el0_svc+0x14/0x40

The offset of mm_user in mm_struct is 0x4c so the NULL pointer is mm.

The concurrency scene can be described as follows: At first, mmget_not_zero() increases the refcount of mm successfully, but then target process is killed. In exit_mm(), the exiting process set

current->mm = NULL;

And when mmput() is called in u2k, tsk->mm is already NULL.

To fix the problem, we use get_task_mm() to keep mm pointer in a local variable.

Signed-off-by: Tang Yizhou tangyizhou@huawei.com Reviewed-by: Ding Tianhong dingtianhong@huawei.com Signed-off-by: Yang Yingliang yangyingliang@huawei.com --- mm/share_pool.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/mm/share_pool.c b/mm/share_pool.c index 04c522cf95d4..f8dfbe80968b 100644 --- a/mm/share_pool.c +++ b/mm/share_pool.c @@ -1809,7 +1809,7 @@ static int sp_hugetlb_entry(pte_t *ptep, unsigned long hmask, * ALIGN(uva+size) - uva_aligned */ static int __sp_walk_page_range(unsigned long uva, unsigned long size, - struct task_struct *tsk, struct sp_walk_data *sp_walk_data) + struct mm_struct *mm, struct sp_walk_data *sp_walk_data) { int ret = 0; struct vm_area_struct *vma; @@ -1830,7 +1830,7 @@ static int __sp_walk_page_range(unsigned long uva, unsigned long size, * In this situation, the correctness of the parameters is mainly * guaranteed by the caller. */ - vma = find_vma(tsk->mm, uva); + vma = find_vma(mm, uva); if (!vma) { if (printk_ratelimit()) pr_err("share pool: u2k input uva %pK is invalid\n", (void *)uva); @@ -1874,7 +1874,7 @@ static int __sp_walk_page_range(unsigned long uva, unsigned long size, } sp_walk_data->pages = pages;

- sp_walk.mm = tsk->mm; + sp_walk.mm = mm; sp_walk.private = sp_walk_data;

ret = walk_page_range(uva_aligned, uva_aligned + size_aligned, @@ -1898,6 +1898,7 @@ void *sp_make_share_u2k(unsigned long uva, unsigned long size, int pid) { int ret = 0; struct task_struct *tsk; + struct mm_struct *mm; void *p = ERR_PTR(-ENODEV); struct sp_walk_data sp_walk_data = { .page_count = 0, @@ -1916,14 +1917,15 @@ void *sp_make_share_u2k(unsigned long uva, unsigned long size, int pid) goto out; }

- if (!mmget_not_zero(tsk->mm)) + mm = get_task_mm(tsk); + if (mm == NULL) goto out_put_task; - down_write(&tsk->mm->mmap_sem); - ret = __sp_walk_page_range(uva, size, tsk, &sp_walk_data); + down_write(&mm->mmap_sem); + ret = __sp_walk_page_range(uva, size, mm, &sp_walk_data); if (ret) { pr_err("share pool: walk page range failed, ret %d\n", ret); - up_write(&tsk->mm->mmap_sem); - mmput(tsk->mm); + up_write(&mm->mmap_sem); + mmput(mm); p = ERR_PTR(ret); goto out_put_task; } @@ -1934,8 +1936,8 @@ void *sp_make_share_u2k(unsigned long uva, unsigned long size, int pid) else p = vmap(sp_walk_data.pages, sp_walk_data.page_count, VM_MAP, PAGE_KERNEL); - up_write(&tsk->mm->mmap_sem); - mmput(tsk->mm); + up_write(&mm->mmap_sem); + mmput(mm);

if (!p) { if (printk_ratelimit()) @@ -2208,6 +2210,7 @@ EXPORT_SYMBOL_GPL(sp_unshare); int sp_walk_page_range(unsigned long uva, unsigned long size, struct task_struct *tsk, struct sp_walk_data *sp_walk_data) { + struct mm_struct *mm; int ret = 0;

if (unlikely(!sp_walk_data)) { @@ -2218,17 +2221,19 @@ int sp_walk_page_range(unsigned long uva, unsigned long size, if (!tsk || (tsk->flags & PF_EXITING)) return -ESRCH;

- sp_walk_data->page_count = 0; - get_task_struct(tsk); - if (!mmget_not_zero(tsk->mm)) { + mm = get_task_mm(tsk); + if (!mm) { put_task_struct(tsk); return -ESRCH; } - down_write(&tsk->mm->mmap_sem); - ret = __sp_walk_page_range(uva, size, tsk, sp_walk_data); - up_write(&tsk->mm->mmap_sem); - mmput(tsk->mm); + + sp_walk_data->page_count = 0; + down_write(&mm->mmap_sem); + ret = __sp_walk_page_range(uva, size, mm, sp_walk_data); + up_write(&mm->mmap_sem); + + mmput(mm); put_task_struct(tsk);

return ret;

From: Jann Horn jannh@google.com

mainline inclusion from mainline-v5.10-rc7 commit c8bcd9c5be24fb9e6132e97da5a35e55a83e36b9 category: bugfix bugzilla: NA CVE: CVE-2020-29660

--------------------------------

Currently, locking of ->session is very inconsistent; most places protect it using the legacy tty mutex, but disassociate_ctty(), __do_SAK(), tiocspgrp() and tiocgsid() don't. Two of the writers hold the ctrl_lock (because they already need it for ->pgrp), but __proc_set_tty() doesn't do that yet.

On a PREEMPT=y system, an unprivileged user can theoretically abuse this broken locking to read 4 bytes of freed memory via TIOCGSID if tiocgsid() is preempted long enough at the right point. (Other things might also go wrong, especially if root-only ioctls are involved; I'm not sure about that.)

Change the locking on ->session such that:

- tty_lock() is held by all writers: By making disassociate_ctty() hold it. This should be fine because the same lock can already be taken through the call to tty_vhangup_session(). The tricky part is that we need to shorten the area covered by siglock to be able to take tty_lock() without ugly retry logic; as far as I can tell, this should be fine, since nothing in the signal_struct is touched in the `if (tty)` branch. - ctrl_lock is held by all writers: By changing __proc_set_tty() to hold the lock a little longer. - All readers that aren't holding tty_lock() hold ctrl_lock: By adding locking to tiocgsid() and __do_SAK(), and expanding the area covered by ctrl_lock in tiocspgrp().

Cc: stable@kernel.org Signed-off-by: Jann Horn jannh@google.com Reviewed-by: Jiri Slaby jirislaby@kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Yang Yingliang yangyingliang@huawei.com Reviewed-by: Jason Yan yanaijie@huawei.com Signed-off-by: Yang Yingliang yangyingliang@huawei.com --- drivers/tty/tty_io.c | 7 ++++++- drivers/tty/tty_jobctrl.c | 44 +++++++++++++++++++++++++++------------ include/linux/tty.h | 4 ++++ 3 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index 503ebfd4b2fe..dd9017f98a31 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -2749,10 +2749,14 @@ void __do_SAK(struct tty_struct *tty) struct task_struct *g, *p; struct pid *session; int i; + unsigned long flags;

if (!tty) return; - session = tty->session; + + spin_lock_irqsave(&tty->ctrl_lock, flags); + session = get_pid(tty->session); + spin_unlock_irqrestore(&tty->ctrl_lock, flags);

tty_ldisc_flush(tty);

@@ -2784,6 +2788,7 @@ void __do_SAK(struct tty_struct *tty) task_unlock(p); } while_each_thread(g, p); read_unlock(&tasklist_lock); + put_pid(session); #endif }

diff --git a/drivers/tty/tty_jobctrl.c b/drivers/tty/tty_jobctrl.c index a42dec3c95d0..ffcab80ba77d 100644 --- a/drivers/tty/tty_jobctrl.c +++ b/drivers/tty/tty_jobctrl.c @@ -103,8 +103,8 @@ static void __proc_set_tty(struct tty_struct *tty) put_pid(tty->session); put_pid(tty->pgrp); tty->pgrp = get_pid(task_pgrp(current)); - spin_unlock_irqrestore(&tty->ctrl_lock, flags); tty->session = get_pid(task_session(current)); + spin_unlock_irqrestore(&tty->ctrl_lock, flags); if (current->signal->tty) { tty_debug(tty, "current tty %s not NULL!!\n", current->signal->tty->name); @@ -293,20 +293,23 @@ void disassociate_ctty(int on_exit) spin_lock_irq(&current->sighand->siglock); put_pid(current->signal->tty_old_pgrp); current->signal->tty_old_pgrp = NULL; - tty = tty_kref_get(current->signal->tty); + spin_unlock_irq(&current->sighand->siglock); + if (tty) { unsigned long flags; + + tty_lock(tty); spin_lock_irqsave(&tty->ctrl_lock, flags); put_pid(tty->session); put_pid(tty->pgrp); tty->session = NULL; tty->pgrp = NULL; spin_unlock_irqrestore(&tty->ctrl_lock, flags); + tty_unlock(tty); tty_kref_put(tty); }

- spin_unlock_irq(&current->sighand->siglock); /* Now clear signal->tty under the lock */ read_lock(&tasklist_lock); session_clear_tty(task_session(current)); @@ -477,14 +480,19 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t return -ENOTTY; if (retval) return retval; - if (!current->signal->tty || - (current->signal->tty != real_tty) || - (real_tty->session != task_session(current))) - return -ENOTTY; + if (get_user(pgrp_nr, p)) return -EFAULT; if (pgrp_nr < 0) return -EINVAL; + + spin_lock_irq(&real_tty->ctrl_lock); + if (!current->signal->tty || + (current->signal->tty != real_tty) || + (real_tty->session != task_session(current))) { + retval = -ENOTTY; + goto out_unlock_ctrl; + } rcu_read_lock(); pgrp = find_vpid(pgrp_nr); retval = -ESRCH; @@ -494,12 +502,12 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t if (session_of_pgrp(pgrp) != task_session(current)) goto out_unlock; retval = 0; - spin_lock_irq(&real_tty->ctrl_lock); put_pid(real_tty->pgrp); real_tty->pgrp = get_pid(pgrp); - spin_unlock_irq(&real_tty->ctrl_lock); out_unlock: rcu_read_unlock(); +out_unlock_ctrl: + spin_unlock_irq(&real_tty->ctrl_lock); return retval; }

@@ -511,20 +519,30 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t * * Obtain the session id of the tty. If there is no session * return an error. - * - * Locking: none. Reference to current->signal->tty is safe. */ static int tiocgsid(struct tty_struct *tty, struct tty_struct *real_tty, pid_t __user *p) { + unsigned long flags; + pid_t sid; + /* * (tty == real_tty) is a cheap way of * testing if the tty is NOT a master pty. */ if (tty == real_tty && current->signal->tty != real_tty) return -ENOTTY; + + spin_lock_irqsave(&real_tty->ctrl_lock, flags); if (!real_tty->session) - return -ENOTTY; - return put_user(pid_vnr(real_tty->session), p); + goto err; + sid = pid_vnr(real_tty->session); + spin_unlock_irqrestore(&real_tty->ctrl_lock, flags); + + return put_user(sid, p); + +err: + spin_unlock_irqrestore(&real_tty->ctrl_lock, flags); + return -ENOTTY; }

/* diff --git a/include/linux/tty.h b/include/linux/tty.h index 74226a8f919c..d808ab9c9aff 100644 --- a/include/linux/tty.h +++ b/include/linux/tty.h @@ -306,6 +306,10 @@ struct tty_struct { struct termiox *termiox; /* May be NULL for unsupported */ char name[64]; struct pid *pgrp; /* Protected by ctrl lock */ + /* + * Writes protected by both ctrl lock and legacy mutex, readers must use + * at least one of them. + */ struct pid *session; unsigned long flags; int count;