| Message ID | 1582528016-2873-1-git-send-email-zhangfei.gao@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | uacce: unmap remaining mmapping from user space | expand |
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 <dave.jiang@intel.com> > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > --- > 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; > }; > > /**
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
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 > . >
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; }; /**
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 <dave.jiang@intel.com> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> --- drivers/misc/uacce/uacce.c | 17 +++++++++++++++++ include/linux/uacce.h | 2 ++ 2 files changed, 19 insertions(+)