[PATCH V2 1/1] device-dax: check for vma range while dax_mmap.

[PATCH V2 1/1] device-dax: check for vma range while dax_mmap.

[Zhang Yi]

This patch prevents a user mapping an illegal vma range that is larger
than a dax device physical resource.

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 | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index 108c37f..6fe8c30 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -177,6 +177,33 @@ 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_ratelimited(dev,
+				"%s: %s: fail, vma range 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)
 {
@@ -469,6 +496,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;
-- 
2.7.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH V2 1/1] device-dax: check for vma range while dax_mmap.

[Verma, Vishal L]

On Mon, 2018-08-13 at 20:02 +0800, Zhang Yi wrote:
> This patch prevents a user mapping an illegal vma range that is larger
> than a dax device physical resource.
> 
> 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 | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 

Looks good to me:
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> index 108c37f..6fe8c30 100644
> --- a/drivers/dax/device.c
> +++ b/drivers/dax/device.c
> @@ -177,6 +177,33 @@ 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_ratelimited(dev,
> +				"%s: %s: fail, vma range 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)
>  {
> @@ -469,6 +496,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;
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH V2 1/1] device-dax: check for vma range while dax_mmap.

[Dave Jiang]

On 08/20/2018 10:53 AM, Verma, Vishal L wrote:
> 
> On Mon, 2018-08-13 at 20:02 +0800, Zhang Yi wrote:
>> This patch prevents a user mapping an illegal vma range that is larger
>> than a dax device physical resource.
>>
>> 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 | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
> 
> Looks good to me:
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

Applied.

> 
>> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
>> index 108c37f..6fe8c30 100644
>> --- a/drivers/dax/device.c
>> +++ b/drivers/dax/device.c
>> @@ -177,6 +177,33 @@ 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_ratelimited(dev,
>> +				"%s: %s: fail, vma range 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)
>>  {
>> @@ -469,6 +496,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;
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH V2 1/1] device-dax: check for vma range while dax_mmap.

[Yi Zhang]

On 2018-08-20 at 12:50:31 -0700, Dave Jiang wrote:
> 
> 
> On 08/20/2018 10:53 AM, Verma, Vishal L wrote:
> > 
> > On Mon, 2018-08-13 at 20:02 +0800, Zhang Yi wrote:
> >> This patch prevents a user mapping an illegal vma range that is larger
> >> than a dax device physical resource.
> >>
> >> 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 | 29 +++++++++++++++++++++++++++++
> >>  1 file changed, 29 insertions(+)
> >>
> > 
> > Looks good to me:
> > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> 
> Applied.
Thanks Dava and Vishal's kindly review. Thank you.
> 
> > 
> >> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> >> index 108c37f..6fe8c30 100644
> >> --- a/drivers/dax/device.c
> >> +++ b/drivers/dax/device.c
> >> @@ -177,6 +177,33 @@ 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_ratelimited(dev,
> >> +				"%s: %s: fail, vma range 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)
> >>  {
> >> @@ -469,6 +496,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;
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH V2 1/1] device-dax: check for vma range while dax_mmap.

[Dan Williams]

On Tue, Aug 21, 2018 at 12:38 AM Yi Zhang <yi.z.zhang@linux.intel.com> wrote:
>
> On 2018-08-20 at 12:50:31 -0700, Dave Jiang wrote:
> >
> >
> > On 08/20/2018 10:53 AM, Verma, Vishal L wrote:
> > >
> > > On Mon, 2018-08-13 at 20:02 +0800, Zhang Yi wrote:
> > >> This patch prevents a user mapping an illegal vma range that is larger
> > >> than a dax device physical resource.
> > >>
> > >> 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 | 29 +++++++++++++++++++++++++++++
> > >>  1 file changed, 29 insertions(+)
> > >>
> > >
> > > Looks good to me:
> > > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> >
> > Applied.
> Thanks Dava and Vishal's kindly review. Thank you.

So, it turns out this patch did not get merged for 4.20. I fumbled it
when returning from vacation. However, I'm not sure it is needed. As
long as attempts to access the out-of-range capacity results in SIGBUS
then the implementation is correct. This is similar to the case where
a file is truncated after the vma is established. That size is
validated at fault time.

Could you be clearer about why this is a problem? The fault sounds
like the correct result.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH V2 1/1] device-dax: check for vma range while dax_mmap.

[Yi Zhang]

On 2018-12-10 at 16:10:31 -0800, Dan Williams wrote:
> On Tue, Aug 21, 2018 at 12:38 AM Yi Zhang <yi.z.zhang@linux.intel.com> wrote:
> >
> > On 2018-08-20 at 12:50:31 -0700, Dave Jiang wrote:
> > >
> > >
> > > On 08/20/2018 10:53 AM, Verma, Vishal L wrote:
> > > >
> > > > On Mon, 2018-08-13 at 20:02 +0800, Zhang Yi wrote:
> > > >> This patch prevents a user mapping an illegal vma range that is larger
> > > >> than a dax device physical resource.
> > > >>
> > > >> 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 | 29 +++++++++++++++++++++++++++++
> > > >>  1 file changed, 29 insertions(+)
> > > >>
> > > >
> > > > Looks good to me:
> > > > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> > >
> > > Applied.
> > Thanks Dava and Vishal's kindly review. Thank you.
> 
> So, it turns out this patch did not get merged for 4.20. I fumbled it
> when returning from vacation. However, I'm not sure it is needed. As
> long as attempts to access the out-of-range capacity results in SIGBUS
> then the implementation is correct. This is similar to the case where
> a file is truncated after the vma is established. That size is
> validated at fault time.
The problem is that we didn't get the fault at we initial the mapping
until attempt to access it, then qemu will failed unexpect without any
output, I think is is better to mention user that we are starting at a 
illegal size, but not faulting at an uncertained time.
> 
> Could you be clearer about why this is a problem? The fault sounds
> like the correct result.
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH V2 1/1] device-dax: check for vma range while dax_mmap.

[Dan Williams]

On Wed, Dec 12, 2018 at 10:12 PM Yi Zhang <yi.z.zhang@linux.intel.com> wrote:
>
> On 2018-12-10 at 16:10:31 -0800, Dan Williams wrote:
> > On Tue, Aug 21, 2018 at 12:38 AM Yi Zhang <yi.z.zhang@linux.intel.com> wrote:
> > >
> > > On 2018-08-20 at 12:50:31 -0700, Dave Jiang wrote:
> > > >
> > > >
> > > > On 08/20/2018 10:53 AM, Verma, Vishal L wrote:
> > > > >
> > > > > On Mon, 2018-08-13 at 20:02 +0800, Zhang Yi wrote:
> > > > >> This patch prevents a user mapping an illegal vma range that is larger
> > > > >> than a dax device physical resource.
> > > > >>
> > > > >> 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 | 29 +++++++++++++++++++++++++++++
> > > > >>  1 file changed, 29 insertions(+)
> > > > >>
> > > > >
> > > > > Looks good to me:
> > > > > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> > > >
> > > > Applied.
> > > Thanks Dava and Vishal's kindly review. Thank you.
> >
> > So, it turns out this patch did not get merged for 4.20. I fumbled it
> > when returning from vacation. However, I'm not sure it is needed. As
> > long as attempts to access the out-of-range capacity results in SIGBUS
> > then the implementation is correct. This is similar to the case where
> > a file is truncated after the vma is established. That size is
> > validated at fault time.
> The problem is that we didn't get the fault at we initial the mapping
> until attempt to access it, then qemu will failed unexpect without any
> output, I think is is better to mention user that we are starting at a
> illegal size, but not faulting at an uncertained time.

That can always happen with mmap'd files. There is no guarantee that a
file range an application successfully mmap'd can be faulted in
without triggering a SIGBUS later. So this change would make
device-dax semantics stricter than regular file semantics. For example
the following program prints "map: pass" and then terminates with
SIGBUS. The "test_data" file is a zero sized file.

int main(void)
{
        int fd = open("test_data", O_RDWR);
        void *addr;

        if (fd < 0)
                return -1;

        addr = mmap(NULL, 1 << 20, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
        printf("map: %s\n", addr == MAP_FAILED ? "fail" : "pass");

        *(char *) addr = 0;

        return 0;
}
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm