Re: [PATCH v3 3/3] uio: fix crash after the device is unregistered

From: Hamish Martin
Date: Thu Jul 05 2018 - 23:40:30 EST

Next message: Alexandre Courbot: "Re: [PATCH v5 00/27] Venus updates"
Previous message: Hamish Martin: "Re: [PATCH v3 2/3] uio: change to use the mutex lock instead of the spin lock"
In reply to: xiubli: "[PATCH v3 3/3] uio: fix crash after the device is unregistered"
Next in thread: Mike Christie: "Re: [PATCH v3 3/3] uio: fix crash after the device is unregistered"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Looks ok to me.

Reviewed-by: Hamish Martin <hamish.martin@xxxxxxxxxxxxxxxxxxx>

On 07/06/2018 02:57 PM, xiubli@xxxxxxxxxx wrote:
> From: Xiubo Li <xiubli@xxxxxxxxxx>
>
> For the target_core_user use case, after the device is unregistered
> it maybe still opened in user space, then the kernel will crash, like:
>
> [ 251.163692] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> [ 251.163820] IP: [<ffffffffc0736213>] show_name+0x23/0x40 [uio]
> [ 251.163965] PGD 8000000062694067 PUD 62696067 PMD 0
> [ 251.164097] Oops: 0000 [#1] SMP
> ...
> [ 251.165605] e1000 mptscsih mptbase drm_panel_orientation_quirks dm_mirror dm_region_hash dm_log dm_mod
> [ 251.166014] CPU: 0 PID: 13380 Comm: tcmu-runner Kdump: loaded Not tainted 3.10.0-916.el7.test.x86_64 #1
> [ 251.166381] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 05/19/2017
> [ 251.166747] task: ffff971eb91db0c0 ti: ffff971e9e384000 task.ti: ffff971e9e384000
> [ 251.167137] RIP: 0010:[<ffffffffc0736213>] [<ffffffffc0736213>] show_name+0x23/0x40 [uio]
> [ 251.167563] RSP: 0018:ffff971e9e387dc8 EFLAGS: 00010282
> [ 251.167978] RAX: 0000000000000000 RBX: ffff971e9e3f8000 RCX: ffff971eb8368d98
> [ 251.168408] RDX: ffff971e9e3f8000 RSI: ffffffffc0738084 RDI: ffff971e9e3f8000
> [ 251.168856] RBP: ffff971e9e387dd0 R08: ffff971eb8bc0018 R09: 0000000000000000
> [ 251.169296] R10: 0000000000001000 R11: ffffffffa09d444d R12: ffffffffa1076e80
> [ 251.169750] R13: ffff971e9e387f18 R14: 0000000000000001 R15: ffff971e9cfb1c80
> [ 251.170213] FS: 00007ff37d175880(0000) GS:ffff971ebb600000(0000) knlGS:0000000000000000
> [ 251.170693] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 251.171248] CR2: 0000000000000008 CR3: 00000000001f6000 CR4: 00000000003607f0
> [ 251.172071] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 251.172640] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 251.173236] Call Trace:
> [ 251.173789] [<ffffffffa0c9b2d3>] dev_attr_show+0x23/0x60
> [ 251.174356] [<ffffffffa0f561b2>] ? mutex_lock+0x12/0x2f
> [ 251.174892] [<ffffffffa0ac6d9f>] sysfs_kf_seq_show+0xcf/0x1f0
> [ 251.175433] [<ffffffffa0ac54e6>] kernfs_seq_show+0x26/0x30
> [ 251.175981] [<ffffffffa0a63be0>] seq_read+0x110/0x3f0
> [ 251.176609] [<ffffffffa0ac5d45>] kernfs_fop_read+0xf5/0x160
> [ 251.177158] [<ffffffffa0a3d3af>] vfs_read+0x9f/0x170
> [ 251.177707] [<ffffffffa0a3e27f>] SyS_read+0x7f/0xf0
> [ 251.178268] [<ffffffffa0f648af>] system_call_fastpath+0x1c/0x21
> [ 251.178823] Code: 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 55 48 89 e5 53 48 89 d3 e8 7e 96 56 e0 48 8b 80 d8 02 00 00 48 89 df 48 c7 c6 84 80 73 c0 <48> 8b 50 08 31 c0 e8 e2 67 44 e0 5b 48 98 5d c3 0f 1f 00 66 2e
> [ 251.180115] RIP [<ffffffffc0736213>] show_name+0x23/0x40 [uio]
> [ 251.180820] RSP <ffff971e9e387dc8>
> [ 251.181473] CR2: 0000000000000008
>
> CC: Hamish Martin <hamish.martin@xxxxxxxxxxxxxxxxxxx>
> CC: Mike Christie <mchristi@xxxxxxxxxx>
> Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> ---
> drivers/uio/uio.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 99 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 655ade4..a0d926e 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -215,7 +215,20 @@ static ssize_t name_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct uio_device *idev = dev_get_drvdata(dev);
> - return sprintf(buf, "%s\n", idev->info->name);
> + int ret;
> +
> + mutex_lock(&idev->info_lock);
> + if (!idev->info) {
> + ret = -EINVAL;
> + dev_err(dev, "the device has been unregistered\n");
> + goto out;
> + }
> +
> + ret = sprintf(buf, "%s\n", idev->info->name);
> +
> +out:
> + mutex_unlock(&idev->info_lock);
> + return ret;
> }
> static DEVICE_ATTR_RO(name);
>
> @@ -223,7 +236,20 @@ static ssize_t version_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct uio_device *idev = dev_get_drvdata(dev);
> - return sprintf(buf, "%s\n", idev->info->version);
> + int ret;
> +
> + mutex_lock(&idev->info_lock);
> + if (!idev->info) {
> + ret = -EINVAL;
> + dev_err(dev, "the device has been unregistered\n");
> + goto out;
> + }
> +
> + ret = sprintf(buf, "%s\n", idev->info->version);
> +
> +out:
> + mutex_unlock(&idev->info_lock);
> + return ret;
> }
> static DEVICE_ATTR_RO(version);
>
> @@ -399,7 +425,12 @@ static void uio_free_minor(struct uio_device *idev)
> */
> void uio_event_notify(struct uio_info *info)
> {
> - struct uio_device *idev = info->uio_dev;
> + struct uio_device *idev;
> +
> + if (!info)
> + return;
> +
> + idev = info->uio_dev;
>
> atomic_inc(&idev->event);
> wake_up_interruptible(&idev->wait);
> @@ -415,11 +446,20 @@ void uio_event_notify(struct uio_info *info)
> static irqreturn_t uio_interrupt(int irq, void *dev_id)
> {
> struct uio_device *idev = (struct uio_device *)dev_id;
> - irqreturn_t ret = idev->info->handler(irq, idev->info);
> + irqreturn_t ret;
> +
> + mutex_lock(&idev->info_lock);
> + if (!idev->info) {
> + ret = IRQ_NONE;
> + goto out;
> + }
>
> + ret = idev->info->handler(irq, idev->info);
> if (ret == IRQ_HANDLED)
> uio_event_notify(idev->info);
>
> +out:
> + mutex_unlock(&idev->info_lock);
> return ret;
> }
>
> @@ -460,6 +500,12 @@ static int uio_open(struct inode *inode, struct file *filep)
> filep->private_data = listener;
>
> mutex_lock(&idev->info_lock);
> + if (!idev->info) {
> + mutex_unlock(&idev->info_lock);
> + ret = -EINVAL;
> + goto err_alloc_listener;
> + }
> +
> if (idev->info && idev->info->open)
> ret = idev->info->open(idev->info, inode);
> mutex_unlock(&idev->info_lock);
> @@ -590,6 +636,11 @@ static ssize_t uio_write(struct file *filep, const char __user *buf,
> s32 irq_on;
>
> mutex_lock(&idev->info_lock);
> + if (!idev->info) {
> + retval = -EINVAL;
> + goto out;
> + }
> +
> if (!idev->info || !idev->info->irq) {
> retval = -EIO;
> goto out;
> @@ -635,10 +686,20 @@ static vm_fault_t uio_vma_fault(struct vm_fault *vmf)
> struct page *page;
> unsigned long offset;
> void *addr;
> + int ret = 0;
> + int mi;
>
> - int mi = uio_find_mem_index(vmf->vma);
> - if (mi < 0)
> - return VM_FAULT_SIGBUS;
> + mutex_lock(&idev->info_lock);
> + if (!idev->info) {
> + ret = VM_FAULT_SIGBUS;
> + goto out;
> + }
> +
> + mi = uio_find_mem_index(vmf->vma);
> + if (mi < 0) {
> + ret = VM_FAULT_SIGBUS;
> + goto out;
> + }
>
> /*
> * We need to subtract mi because userspace uses offset = N*PAGE_SIZE
> @@ -653,7 +714,11 @@ static vm_fault_t uio_vma_fault(struct vm_fault *vmf)
> page = vmalloc_to_page(addr);
> get_page(page);
> vmf->page = page;
> - return 0;
> +
> +out:
> + mutex_unlock(&idev->info_lock);
> +
> + return ret;
> }
>
> static const struct vm_operations_struct uio_logical_vm_ops = {
> @@ -678,6 +743,7 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
> struct uio_device *idev = vma->vm_private_data;
> int mi = uio_find_mem_index(vma);
> struct uio_mem *mem;
> +
> if (mi < 0)
> return -EINVAL;
> mem = idev->info->mem + mi;
> @@ -719,30 +785,46 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
>
> vma->vm_private_data = idev;
>
> + mutex_lock(&idev->info_lock);
> + if (!idev->info) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> mi = uio_find_mem_index(vma);
> - if (mi < 0)
> - return -EINVAL;
> + if (mi < 0) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> requested_pages = vma_pages(vma);
> actual_pages = ((idev->info->mem[mi].addr & ~PAGE_MASK)
> + idev->info->mem[mi].size + PAGE_SIZE -1) >> PAGE_SHIFT;
> - if (requested_pages > actual_pages)
> - return -EINVAL;
> + if (requested_pages > actual_pages) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> if (idev->info->mmap) {
> ret = idev->info->mmap(idev->info, vma);
> - return ret;
> + goto out;
> }
>
> switch (idev->info->mem[mi].memtype) {
> case UIO_MEM_PHYS:
> - return uio_mmap_physical(vma);
> + ret = uio_mmap_physical(vma);
> + break;
> case UIO_MEM_LOGICAL:
> case UIO_MEM_VIRTUAL:
> - return uio_mmap_logical(vma);
> + ret = uio_mmap_logical(vma);
> + break;
> default:
> - return -EINVAL;
> + ret = -EINVAL;
> }
> +
> +out:
> + mutex_unlock(&idev->info_lock);
> + return 0;
> }
>
> static const struct file_operations uio_fops = {
> @@ -932,12 +1014,12 @@ void uio_unregister_device(struct uio_info *info)
>
> uio_free_minor(idev);
>
> + mutex_lock(&idev->info_lock);
> uio_dev_del_attributes(idev);
>
> if (info->irq && info->irq != UIO_IRQ_CUSTOM)
> free_irq(info->irq, idev);
>
> - mutex_lock(&idev->info_lock);
> idev->info = NULL;
> mutex_unlock(&idev->info_lock);
>

Next message: Alexandre Courbot: "Re: [PATCH v5 00/27] Venus updates"
Previous message: Hamish Martin: "Re: [PATCH v3 2/3] uio: change to use the mutex lock instead of the spin lock"
In reply to: xiubli: "[PATCH v3 3/3] uio: fix crash after the device is unregistered"
Next in thread: Mike Christie: "Re: [PATCH v3 3/3] uio: fix crash after the device is unregistered"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]