Re: [PATCH] uacce: unmap remaining mmapping from user space
Hi, On 2020/2/25 16:33, zhangfei wrote: Hi, Zaibo On 2020/2/24 下午3:17, Xu Zaibo wrote: @@ -585,6 +595,13 @@ void uacce_remove(struct uacce_device *uacce) cdev_device_del(uacce->cdev, &uacce->dev); xa_erase(&uacce_xa, uacce->dev_id); put_device(&uacce->dev); + +/* + * unmap remainning mapping from user space, preventing user still + * access the mmaped area while parent device is already removed + */ +if (uacce->inode) +unmap_mapping_range(uacce->inode->i_mapping, 0, 0, 1); Should we unmap them at the first of 'uacce_remove', and before 'uacce_put_queue'? We can do this, Though it does not matter, since user space can not interrupt kernel function uacce_remove. I think it matters :) Image that the process holds the uacce queue is running(read and write the queue), then you do 'uacce_remove'. The process is running(read and write the queue) well in the time between 'uacce_put_queue' and 'unmap_mapping_range', however, the queue with its DMA memory may be gotten and used by other guys in this time, since you have released them in kernel. As a result, the running process will be a disaster. cheers, Zaibo . Thanks . ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] uacce: unmap remaining mmapping from user space
Hi, Zaibo On 2020/2/24 下午3:17, Xu Zaibo wrote: @@ -585,6 +595,13 @@ void uacce_remove(struct uacce_device *uacce) cdev_device_del(uacce->cdev, &uacce->dev); xa_erase(&uacce_xa, uacce->dev_id); put_device(&uacce->dev); + + /* + * unmap remainning mapping from user space, preventing user still + * access the mmaped area while parent device is already removed + */ + if (uacce->inode) + unmap_mapping_range(uacce->inode->i_mapping, 0, 0, 1); Should we unmap them at the first of 'uacce_remove', and before 'uacce_put_queue'? We can do this, Though it does not matter, since user space can not interrupt kernel function uacce_remove. Thanks ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] uacce: unmap remaining mmapping from user space
Hi Zhangfei, On 2020/2/24 15:06, Zhangfei Gao wrote: When uacce parent device module is removed, user app may still keep the mmaped area, which can be accessed unsafely. When rmmod, Parent device drvier will call uacce_remove, which unmap all remaining mapping from user space for safety. VM_FAULT_SIGBUS is also reported to user space accordingly. Suggested-by: Dave Jiang Signed-off-by: Zhangfei Gao --- drivers/misc/uacce/uacce.c | 17 + include/linux/uacce.h | 2 ++ 2 files changed, 19 insertions(+) diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c index ffced4d..1bcc5e6 100644 --- a/drivers/misc/uacce/uacce.c +++ b/drivers/misc/uacce/uacce.c @@ -224,6 +224,7 @@ static int uacce_fops_open(struct inode *inode, struct file *filep) init_waitqueue_head(&q->wait); filep->private_data = q; + uacce->inode = inode; q->state = UACCE_Q_INIT; return 0; @@ -253,6 +254,14 @@ static int uacce_fops_release(struct inode *inode, struct file *filep) return 0; } +static vm_fault_t uacce_vma_fault(struct vm_fault *vmf) +{ + if (vmf->flags & (FAULT_FLAG_MKWRITE | FAULT_FLAG_WRITE)) + return VM_FAULT_SIGBUS; + + return 0; +} + static void uacce_vma_close(struct vm_area_struct *vma) { struct uacce_queue *q = vma->vm_private_data; @@ -265,6 +274,7 @@ static void uacce_vma_close(struct vm_area_struct *vma) } static const struct vm_operations_struct uacce_vm_ops = { + .fault = uacce_vma_fault, .close = uacce_vma_close, }; @@ -585,6 +595,13 @@ void uacce_remove(struct uacce_device *uacce) cdev_device_del(uacce->cdev, &uacce->dev); xa_erase(&uacce_xa, uacce->dev_id); put_device(&uacce->dev); + + /* +* unmap remainning mapping from user space, preventing user still +* access the mmaped area while parent device is already removed +*/ + if (uacce->inode) + unmap_mapping_range(uacce->inode->i_mapping, 0, 0, 1); Should we unmap them at the first of 'uacce_remove', and before 'uacce_put_queue'? Thanks, Zaibo . } EXPORT_SYMBOL_GPL(uacce_remove); diff --git a/include/linux/uacce.h b/include/linux/uacce.h index 904a461..0e215e6 100644 --- a/include/linux/uacce.h +++ b/include/linux/uacce.h @@ -98,6 +98,7 @@ struct uacce_queue { * @priv: private pointer of the uacce * @mm_list: list head of uacce_mm->list * @mm_lock: lock for mm_list + * @inode: core vfs */ struct uacce_device { const char *algs; @@ -113,6 +114,7 @@ struct uacce_device { void *priv; struct list_head mm_list; struct mutex mm_lock; + struct inode *inode; }; /** ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] uacce: unmap remaining mmapping from user space
When uacce parent device module is removed, user app may still keep the mmaped area, which can be accessed unsafely. When rmmod, Parent device drvier will call uacce_remove, which unmap all remaining mapping from user space for safety. VM_FAULT_SIGBUS is also reported to user space accordingly. Suggested-by: Dave Jiang Signed-off-by: Zhangfei Gao --- drivers/misc/uacce/uacce.c | 17 + include/linux/uacce.h | 2 ++ 2 files changed, 19 insertions(+) diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c index ffced4d..1bcc5e6 100644 --- a/drivers/misc/uacce/uacce.c +++ b/drivers/misc/uacce/uacce.c @@ -224,6 +224,7 @@ static int uacce_fops_open(struct inode *inode, struct file *filep) init_waitqueue_head(&q->wait); filep->private_data = q; + uacce->inode = inode; q->state = UACCE_Q_INIT; return 0; @@ -253,6 +254,14 @@ static int uacce_fops_release(struct inode *inode, struct file *filep) return 0; } +static vm_fault_t uacce_vma_fault(struct vm_fault *vmf) +{ + if (vmf->flags & (FAULT_FLAG_MKWRITE | FAULT_FLAG_WRITE)) + return VM_FAULT_SIGBUS; + + return 0; +} + static void uacce_vma_close(struct vm_area_struct *vma) { struct uacce_queue *q = vma->vm_private_data; @@ -265,6 +274,7 @@ static void uacce_vma_close(struct vm_area_struct *vma) } static const struct vm_operations_struct uacce_vm_ops = { + .fault = uacce_vma_fault, .close = uacce_vma_close, }; @@ -585,6 +595,13 @@ void uacce_remove(struct uacce_device *uacce) cdev_device_del(uacce->cdev, &uacce->dev); xa_erase(&uacce_xa, uacce->dev_id); put_device(&uacce->dev); + + /* +* unmap remainning mapping from user space, preventing user still +* access the mmaped area while parent device is already removed +*/ + if (uacce->inode) + unmap_mapping_range(uacce->inode->i_mapping, 0, 0, 1); } EXPORT_SYMBOL_GPL(uacce_remove); diff --git a/include/linux/uacce.h b/include/linux/uacce.h index 904a461..0e215e6 100644 --- a/include/linux/uacce.h +++ b/include/linux/uacce.h @@ -98,6 +98,7 @@ struct uacce_queue { * @priv: private pointer of the uacce * @mm_list: list head of uacce_mm->list * @mm_lock: lock for mm_list + * @inode: core vfs */ struct uacce_device { const char *algs; @@ -113,6 +114,7 @@ struct uacce_device { void *priv; struct list_head mm_list; struct mutex mm_lock; + struct inode *inode; }; /** -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu