[PATCH] NVMe: Add a buffer length parameter to struct nvme_user_io. Check buffer length in nvme_submit_io to avoid buffer overflow.

[PATCH] NVMe: Add a buffer length parameter to struct nvme_user_io. Check buffer length in nvme_submit_io to avoid buffer overflow.

[David Darrington]

Added a buffer length parameter to struct nvme_user_io so that
nvme_submit_io can prevent writing past the end of the user buffer.

Signed-off-by: David Darrington <david.darrington at hgst.com>
---
 drivers/block/nvme-core.c |    6 +++++-
 include/uapi/linux/nvme.h |    4 +++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index da52092..2aa4346 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1381,6 +1381,10 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 	if (copy_from_user(&io, uio, sizeof(io)))
 		return -EFAULT;
 	length = (io.nblocks + 1) << ns->lba_shift;
+
+	if (io.dxfer_len < length)
+		return -EINVAL;
+
 	meta_len = (io.nblocks + 1) * ns->ms;
 
 	if (meta_len && ((io.metadata & 3) || !io.metadata))
@@ -1390,7 +1394,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 	case nvme_cmd_write:
 	case nvme_cmd_read:
 	case nvme_cmd_compare:
-		iod = nvme_map_user_pages(dev, io.opcode & 1, io.addr, length);
+		iod = nvme_map_user_pages(dev, io.opcode & 1, io.dxferp, length);
 		break;
 	default:
 		return -EINVAL;
diff --git a/include/uapi/linux/nvme.h b/include/uapi/linux/nvme.h
index 989c04e..40b5b52 100644
--- a/include/uapi/linux/nvme.h
+++ b/include/uapi/linux/nvme.h
@@ -441,7 +441,9 @@ struct nvme_user_io {
 	__u16	nblocks;
 	__u16	rsvd;
 	__u64	metadata;
-	__u64	addr;
+	__u32	rsvd1;
+	__u32	dxfer_len;	/* length of data xfer buffer */
+	__u64	dxferp;		/* pointer to data xfer buffer */
 	__u64	slba;
 	__u32	dsmgmt;
 	__u32	reftag;
-- 
1.7.1

[PATCH] NVMe: Add a buffer length parameter to struct nvme_user_io. Check buffer length in nvme_submit_io to avoid buffer overflow.

[Keith Busch]

On Mon, 4 Nov 2013, David Darrington wrote:

> Added a buffer length parameter to struct nvme_user_io so that
> nvme_submit_io can prevent writing past the end of the user buffer.

This extra check seems redundant. Doesn't get_user_pages_fast already
fail if the user buffer is too small?

>
> Signed-off-by: David Darrington <david.darrington at hgst.com>
> ---
> drivers/block/nvme-core.c |    6 +++++-
> include/uapi/linux/nvme.h |    4 +++-
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> index da52092..2aa4346 100644
> --- a/drivers/block/nvme-core.c
> +++ b/drivers/block/nvme-core.c
> @@ -1381,6 +1381,10 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
> 	if (copy_from_user(&io, uio, sizeof(io)))
> 		return -EFAULT;
> 	length = (io.nblocks + 1) << ns->lba_shift;
> +
> +	if (io.dxfer_len < length)
> +		return -EINVAL;
> +
> 	meta_len = (io.nblocks + 1) * ns->ms;
>
> 	if (meta_len && ((io.metadata & 3) || !io.metadata))
> @@ -1390,7 +1394,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
> 	case nvme_cmd_write:
> 	case nvme_cmd_read:
> 	case nvme_cmd_compare:
> -		iod = nvme_map_user_pages(dev, io.opcode & 1, io.addr, length);
> +		iod = nvme_map_user_pages(dev, io.opcode & 1, io.dxferp, length);
> 		break;
> 	default:
> 		return -EINVAL;
> diff --git a/include/uapi/linux/nvme.h b/include/uapi/linux/nvme.h
> index 989c04e..40b5b52 100644
> --- a/include/uapi/linux/nvme.h
> +++ b/include/uapi/linux/nvme.h
> @@ -441,7 +441,9 @@ struct nvme_user_io {
> 	__u16	nblocks;
> 	__u16	rsvd;
> 	__u64	metadata;
> -	__u64	addr;
> +	__u32	rsvd1;
> +	__u32	dxfer_len;	/* length of data xfer buffer */
> +	__u64	dxferp;		/* pointer to data xfer buffer */
> 	__u64	slba;
> 	__u32	dsmgmt;
> 	__u32	reftag;
> -- 
> 1.7.1
>
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://merlin.infradead.org/mailman/listinfo/linux-nvme
>

[PATCH] NVMe: Add a buffer length parameter to struct nvme_user_io. Check buffer length in nvme_submit_io to avoid buffer overflow.

[David.Darrington]

The patch was for a reported problem. I have a testcase that segfaults 
after the user buffer is over written. I created a request to read 1 LBA, 
and passed in a buffer of less than 512 bytes. The driver wrote past the 
buffer, corrupting storage leading to the segfault. There is a similar 
param and length check in the sg ioctl path. Look near the end of 
nvme_trans_io. In my case, the buffer is allocated on the stack, as a 
local variable. that might explain why get_user_pages does not catch the 
mistake.





 

"Linux-nvme" <linux-nvme-bounces at lists.infradead.org> wrote on 11/04/2013 
11:27:29 AM:

> On Mon, 4 Nov 2013, David Darrington wrote:
> 
> > Added a buffer length parameter to struct nvme_user_io so that
> > nvme_submit_io can prevent writing past the end of the user buffer.
> 
> This extra check seems redundant. Doesn't get_user_pages_fast already
> fail if the user buffer is too small?
> 
> >
> > Signed-off-by: David Darrington <david.darrington at hgst.com>
> > ---
> > drivers/block/nvme-core.c |    6 +++++-
> > include/uapi/linux/nvme.h |    4 +++-
> > 2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> > index da52092..2aa4346 100644
> > --- a/drivers/block/nvme-core.c
> > +++ b/drivers/block/nvme-core.c
> > @@ -1381,6 +1381,10 @@ static int nvme_submit_io(struct nvme_ns 
> *ns, struct nvme_user_io __user *uio)
> >    if (copy_from_user(&io, uio, sizeof(io)))
> >       return -EFAULT;
> >    length = (io.nblocks + 1) << ns->lba_shift;
> > +
> > +   if (io.dxfer_len < length)
> > +      return -EINVAL;
> > +
> >    meta_len = (io.nblocks + 1) * ns->ms;
> >
> >    if (meta_len && ((io.metadata & 3) || !io.metadata))
> > @@ -1390,7 +1394,7 @@ static int nvme_submit_io(struct nvme_ns 
> *ns, struct nvme_user_io __user *uio)
> >    case nvme_cmd_write:
> >    case nvme_cmd_read:
> >    case nvme_cmd_compare:
> > -      iod = nvme_map_user_pages(dev, io.opcode & 1, io.addr, length);
> > +      iod = nvme_map_user_pages(dev, io.opcode & 1, io.dxferp, 
length);
> >       break;
> >    default:
> >       return -EINVAL;
> > diff --git a/include/uapi/linux/nvme.h b/include/uapi/linux/nvme.h
> > index 989c04e..40b5b52 100644
> > --- a/include/uapi/linux/nvme.h
> > +++ b/include/uapi/linux/nvme.h
> > @@ -441,7 +441,9 @@ struct nvme_user_io {
> >    __u16   nblocks;
> >    __u16   rsvd;
> >    __u64   metadata;
> > -   __u64   addr;
> > +   __u32   rsvd1;
> > +   __u32   dxfer_len;   /* length of data xfer buffer */
> > +   __u64   dxferp;      /* pointer to data xfer buffer */
> >    __u64   slba;
> >    __u32   dsmgmt;
> >    __u32   reftag;
> > -- 
> > 1.7.1
> >
> >
> > _______________________________________________
> > Linux-nvme mailing list
> > Linux-nvme at lists.infradead.org
> > http://merlin.infradead.org/mailman/listinfo/linux-nvme
> >
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://merlin.infradead.org/mailman/listinfo/linux-nvme

[PATCH] NVMe: Add a buffer length parameter to struct nvme_user_io. Check buffer length in nvme_submit_io to avoid buffer overflow.

[Matthew Wilcox]

On Mon, Nov 04, 2013@11:02:36AM -0600, David Darrington wrote:
> @@ -441,7 +441,9 @@ struct nvme_user_io {
>  	__u16	nblocks;
>  	__u16	rsvd;
>  	__u64	metadata;
> -	__u64	addr;
> +	__u32	rsvd1;
> +	__u32	dxfer_len;	/* length of data xfer buffer */
> +	__u64	dxferp;		/* pointer to data xfer buffer */
>  	__u64	slba;
>  	__u32	dsmgmt;
>  	__u32	reftag;

You can't just change the size of nvme_user_io; that breaks the ABI.
We'd need a _V2 version of the ioctl or something if we actually need
to change it.

[PATCH] NVMe: Add a buffer length parameter to struct nvme_user_io. Check buffer length in nvme_submit_io to avoid buffer overflow.

[David.Darrington]

So we have these choices:

1. leave the code as is, with a known buffer overflow problem.
2. create a _V2 version of the ioctl (in which case the v1 version still 
has the issue)
3. Fix the problem in some other way.

Perhaps 1) is not as bad as it sounds. Even if we fix the problem, a pgm 
could pass in the wrong length and get the same result. 

The SG_IO ioctl does include a length, so at least the fix is consistent.




Matthew Wilcox <willy at linux.intel.com> 
Sent by: "Linux-nvme" <linux-nvme-bounces at lists.infradead.org>
11/04/2013 12:26 PM

To
David Darrington <david.darrington at hgst.com>
cc
linux-nvme at lists.infradead.org
Subject
Re: [PATCH] NVMe: Add a buffer length parameter to struct nvme_user_io. 
Check buffer length in nvme_submit_io to avoid buffer overflow.






On Mon, Nov 04, 2013@11:02:36AM -0600, David Darrington wrote:
> @@ -441,7 +441,9 @@ struct nvme_user_io {
>                __u16           nblocks;
>                __u16           rsvd;
>                __u64           metadata;
> -              __u64           addr;
> +              __u32           rsvd1;
> +              __u32           dxfer_len;              /* length of data 
xfer buffer */
> +              __u64           dxferp; /* pointer to data xfer buffer */
>                __u64           slba;
>                __u32           dsmgmt;
>                __u32           reftag;

You can't just change the size of nvme_user_io; that breaks the ABI.
We'd need a _V2 version of the ioctl or something if we actually need
to change it.

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://merlin.infradead.org/mailman/listinfo/linux-nvme