| Message ID | e260d513ab831bfe1298374c87c6cca5f7dd3a6f.1533036772.git.yi.z.zhang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/1] device-dax: check for vma range while dax_mmap. | expand |
On 07/31/2018 04:46 AM, Zhang Yi wrote: > It should be prevent user map an illegal vma range which larger than > dax device phiscal resourse, as we don't have swap logic while page > faulting in dax device. This patch prevents a user mapping an illegal vma range that is larger than a dax device physical resource. > > Applications, especailly qemu, map the /dev/dax for virtual nvdimm's > backend device, we defined the v-nvdimm label area at the end of mapped > rang. By using an illegal size that exceeds the physical resource of > /dev/dax, then it will triger qemu a signal fault while accessing these > label area. When qemu maps the dax device for virtual nvdimm's backend device, the v-nvdimm label area is defined at the end of mapped range. By using an illegal size that exceeds the range of the device dax, it will trigger a fault with qemu. > > Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com> > --- > drivers/dax/device.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/drivers/dax/device.c b/drivers/dax/device.c > index aff2c15..c9a50cd 100644 > --- a/drivers/dax/device.c > +++ b/drivers/dax/device.c > @@ -177,6 +177,32 @@ static const struct attribute_group *dax_attribute_groups[] = { > NULL, > }; > > +static int check_vma_range(struct dev_dax *dev_dax, struct vm_area_struct *vma, > + const char *func) > +{ > + struct device *dev = &dev_dax->dev; > + struct resource *res; > + unsigned long size; > + int ret, i; > + > + if (!dax_alive(dev_dax->dax_dev)) > + return -ENXIO; > + > + size = vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT); > + ret = -EINVAL; > + for (i = 0; i < dev_dax->num_resources; i++) { > + res = &dev_dax->res[i]; > + if (size > resource_size(res)) { > + dev_info(dev, "%s: %s: fail, vma range is overflow\n", > + current->comm, func); > + ret = -EINVAL; > + continue; > + } else > + return 0; > + } > + return ret; > +} > + > static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma, > const char *func) > { > @@ -465,6 +491,8 @@ static int dax_mmap(struct file *filp, struct vm_area_struct *vma) > */ > id = dax_read_lock(); > rc = check_vma(dev_dax, vma, __func__); > + if (!rc) > + rc |= check_vma_range(dev_dax, vma, __func__); I think you want to augment check_vma() rather than adding another function? If this is added inside check_vma() then you can also skip the !dax_alive() check. Do you expect this function to be called anywhere else? > dax_read_unlock(id); > if (rc) > return rc; >
On 2018年08月02日 03:40, Dave Jiang wrote: > > On 07/31/2018 04:46 AM, Zhang Yi wrote: >> It should be prevent user map an illegal vma range which larger than >> dax device phiscal resourse, as we don't have swap logic while page >> faulting in dax device. > This patch prevents a user mapping an illegal vma range that is larger > than a dax device physical resource. > >> Applications, especailly qemu, map the /dev/dax for virtual nvdimm's >> backend device, we defined the v-nvdimm label area at the end of mapped >> rang. By using an illegal size that exceeds the physical resource of >> /dev/dax, then it will triger qemu a signal fault while accessing these >> label area. > When qemu maps the dax device for virtual nvdimm's backend device, the > v-nvdimm label area is defined at the end of mapped range. By using an > illegal size that exceeds the range of the device dax, it will trigger a > fault with qemu. Thanks Dava, that's could be much better. > >> Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com> >> --- >> drivers/dax/device.c | 28 ++++++++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/drivers/dax/device.c b/drivers/dax/device.c >> index aff2c15..c9a50cd 100644 >> --- a/drivers/dax/device.c >> +++ b/drivers/dax/device.c >> @@ -177,6 +177,32 @@ static const struct attribute_group *dax_attribute_groups[] = { >> NULL, >> }; >> >> +static int check_vma_range(struct dev_dax *dev_dax, struct vm_area_struct *vma, >> + const char *func) >> +{ >> + struct device *dev = &dev_dax->dev; >> + struct resource *res; >> + unsigned long size; >> + int ret, i; >> + >> + if (!dax_alive(dev_dax->dax_dev)) >> + return -ENXIO; >> + >> + size = vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT); >> + ret = -EINVAL; >> + for (i = 0; i < dev_dax->num_resources; i++) { >> + res = &dev_dax->res[i]; >> + if (size > resource_size(res)) { >> + dev_info(dev, "%s: %s: fail, vma range is overflow\n", >> + current->comm, func); >> + ret = -EINVAL; >> + continue; >> + } else >> + return 0; >> + } >> + return ret; >> +} >> + >> static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma, >> const char *func) >> { >> @@ -465,6 +491,8 @@ static int dax_mmap(struct file *filp, struct vm_area_struct *vma) >> */ >> id = dax_read_lock(); >> rc = check_vma(dev_dax, vma, __func__); >> + if (!rc) >> + rc |= check_vma_range(dev_dax, vma, __func__); > I think you want to augment check_vma() rather than adding another > function? If this is added inside check_vma() then you can also skip the > !dax_alive() check. Do you expect this function to be called anywhere else? since check_vma range also will be called while dax page faulting. I don't wanna this check_vma_range introduce some additional workload while page fault. just let it checked in the mmap scenario > >> dax_read_unlock(id); >> if (rc) >> return rc; >>
On 08/02/2018 02:32 AM, Zhang,Yi wrote: > > > On 2018年08月02日 03:40, Dave Jiang wrote: >> >> On 07/31/2018 04:46 AM, Zhang Yi wrote: >>> It should be prevent user map an illegal vma range which larger than >>> dax device phiscal resourse, as we don't have swap logic while page >>> faulting in dax device. >> This patch prevents a user mapping an illegal vma range that is larger >> than a dax device physical resource. >> >>> Applications, especailly qemu, map the /dev/dax for virtual nvdimm's >>> backend device, we defined the v-nvdimm label area at the end of mapped >>> rang. By using an illegal size that exceeds the physical resource of >>> /dev/dax, then it will triger qemu a signal fault while accessing these >>> label area. >> When qemu maps the dax device for virtual nvdimm's backend device, the >> v-nvdimm label area is defined at the end of mapped range. By using an >> illegal size that exceeds the range of the device dax, it will trigger a >> fault with qemu. > Thanks Dava, that's could be much better. >> >>> Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com> >>> --- >>> drivers/dax/device.c | 28 ++++++++++++++++++++++++++++ >>> 1 file changed, 28 insertions(+) >>> >>> diff --git a/drivers/dax/device.c b/drivers/dax/device.c >>> index aff2c15..c9a50cd 100644 >>> --- a/drivers/dax/device.c >>> +++ b/drivers/dax/device.c >>> @@ -177,6 +177,32 @@ static const struct attribute_group *dax_attribute_groups[] = { >>> NULL, >>> }; >>> >>> +static int check_vma_range(struct dev_dax *dev_dax, struct vm_area_struct *vma, >>> + const char *func) >>> +{ >>> + struct device *dev = &dev_dax->dev; >>> + struct resource *res; >>> + unsigned long size; >>> + int ret, i; >>> + >>> + if (!dax_alive(dev_dax->dax_dev)) >>> + return -ENXIO; >>> + >>> + size = vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT); >>> + ret = -EINVAL; >>> + for (i = 0; i < dev_dax->num_resources; i++) { >>> + res = &dev_dax->res[i]; >>> + if (size > resource_size(res)) { >>> + dev_info(dev, "%s: %s: fail, vma range is overflow\n", >>> + current->comm, func); >>> + ret = -EINVAL; >>> + continue; >>> + } else >>> + return 0; >>> + } >>> + return ret; >>> +} >>> + >>> static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma, >>> const char *func) >>> { >>> @@ -465,6 +491,8 @@ static int dax_mmap(struct file *filp, struct vm_area_struct *vma) >>> */ >>> id = dax_read_lock(); >>> rc = check_vma(dev_dax, vma, __func__); >>> + if (!rc) >>> + rc |= check_vma_range(dev_dax, vma, __func__); I don't see any reason to logical or the return code. It should be 0, so you can just assign it to check_vma_range(). >> I think you want to augment check_vma() rather than adding another >> function? If this is added inside check_vma() then you can also skip the >> !dax_alive() check. Do you expect this function to be called anywhere else? > since check_vma range also will be called while dax page faulting. I > don't wanna this check_vma_range introduce some additional workload > while page fault. just let it checked in the mmap scenario Good point. Ok. >> >>> dax_read_unlock(id); >>> if (rc) >>> return rc; >>> >
On 2018年08月08日 08:05, Dave Jiang wrote: > > On 08/02/2018 02:32 AM, Zhang,Yi wrote: >> >> On 2018年08月02日 03:40, Dave Jiang wrote: >>> On 07/31/2018 04:46 AM, Zhang Yi wrote: >>>> It should be prevent user map an illegal vma range which larger than >>>> dax device phiscal resourse, as we don't have swap logic while page >>>> faulting in dax device. >>> This patch prevents a user mapping an illegal vma range that is larger >>> than a dax device physical resource. >>> >>>> Applications, especailly qemu, map the /dev/dax for virtual nvdimm's >>>> backend device, we defined the v-nvdimm label area at the end of mapped >>>> rang. By using an illegal size that exceeds the physical resource of >>>> /dev/dax, then it will triger qemu a signal fault while accessing these >>>> label area. >>> When qemu maps the dax device for virtual nvdimm's backend device, the >>> v-nvdimm label area is defined at the end of mapped range. By using an >>> illegal size that exceeds the range of the device dax, it will trigger a >>> fault with qemu. >> Thanks Dava, that's could be much better. >>>> Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com> >>>> --- >>>> drivers/dax/device.c | 28 ++++++++++++++++++++++++++++ >>>> 1 file changed, 28 insertions(+) >>>> >>>> diff --git a/drivers/dax/device.c b/drivers/dax/device.c >>>> index aff2c15..c9a50cd 100644 >>>> --- a/drivers/dax/device.c >>>> +++ b/drivers/dax/device.c >>>> @@ -177,6 +177,32 @@ static const struct attribute_group *dax_attribute_groups[] = { >>>> NULL, >>>> }; >>>> >>>> +static int check_vma_range(struct dev_dax *dev_dax, struct vm_area_struct *vma, >>>> + const char *func) >>>> +{ >>>> + struct device *dev = &dev_dax->dev; >>>> + struct resource *res; >>>> + unsigned long size; >>>> + int ret, i; >>>> + >>>> + if (!dax_alive(dev_dax->dax_dev)) >>>> + return -ENXIO; >>>> + >>>> + size = vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT); >>>> + ret = -EINVAL; >>>> + for (i = 0; i < dev_dax->num_resources; i++) { >>>> + res = &dev_dax->res[i]; >>>> + if (size > resource_size(res)) { >>>> + dev_info(dev, "%s: %s: fail, vma range is overflow\n", >>>> + current->comm, func); >>>> + ret = -EINVAL; >>>> + continue; >>>> + } else >>>> + return 0; >>>> + } >>>> + return ret; >>>> +} >>>> + >>>> static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma, >>>> const char *func) >>>> { >>>> @@ -465,6 +491,8 @@ static int dax_mmap(struct file *filp, struct vm_area_struct *vma) >>>> */ >>>> id = dax_read_lock(); >>>> rc = check_vma(dev_dax, vma, __func__); >>>> + if (!rc) >>>> + rc |= check_vma_range(dev_dax, vma, __func__); > I don't see any reason to logical or the return code. It should be 0, so > you can just assign it to check_vma_range(). Ah.. Yes, Thanks for your kindly review, Dace, I will send the next version. > >>> I think you want to augment check_vma() rather than adding another >>> function? If this is added inside check_vma() then you can also skip the >>> !dax_alive() check. Do you expect this function to be called anywhere else? >> since check_vma range also will be called while dax page faulting. I >> don't wanna this check_vma_range introduce some additional workload >> while page fault. just let it checked in the mmap scenario > Good point. Ok. > >>>> dax_read_unlock(id); >>>> if (rc) >>>> return rc; >>>>
diff --git a/drivers/dax/device.c b/drivers/dax/device.c index aff2c15..c9a50cd 100644 --- a/drivers/dax/device.c +++ b/drivers/dax/device.c @@ -177,6 +177,32 @@ static const struct attribute_group *dax_attribute_groups[] = { NULL, }; +static int check_vma_range(struct dev_dax *dev_dax, struct vm_area_struct *vma, + const char *func) +{ + struct device *dev = &dev_dax->dev; + struct resource *res; + unsigned long size; + int ret, i; + + if (!dax_alive(dev_dax->dax_dev)) + return -ENXIO; + + size = vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT); + ret = -EINVAL; + for (i = 0; i < dev_dax->num_resources; i++) { + res = &dev_dax->res[i]; + if (size > resource_size(res)) { + dev_info(dev, "%s: %s: fail, vma range is overflow\n", + current->comm, func); + ret = -EINVAL; + continue; + } else + return 0; + } + return ret; +} + static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma, const char *func) { @@ -465,6 +491,8 @@ static int dax_mmap(struct file *filp, struct vm_area_struct *vma) */ id = dax_read_lock(); rc = check_vma(dev_dax, vma, __func__); + if (!rc) + rc |= check_vma_range(dev_dax, vma, __func__); dax_read_unlock(id); if (rc) return rc;
It should be prevent user map an illegal vma range which larger than dax device phiscal resourse, as we don't have swap logic while page faulting in dax device. Applications, especailly qemu, map the /dev/dax for virtual nvdimm's backend device, we defined the v-nvdimm label area at the end of mapped rang. By using an illegal size that exceeds the physical resource of /dev/dax, then it will triger qemu a signal fault while accessing these label area. Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com> --- drivers/dax/device.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)