[dpdk-dev] [PATCH 0/8] vhost: input validation enhancements

[dpdk-dev] [PATCH 0/8] vhost: input validation enhancements

[Stefan Hajnoczi]

This patch series addresses missing input validation that I came across when
reviewing vhost_user.c.

The first patch explains the security model and the rest fixes places with
missing checks.

Now is a good time to discuss the security model if anyone disagrees or has
questions about what Patch 1 says.

Stefan Hajnoczi (8):
  vhost: add security model documentation to vhost_user.c
  vhost: avoid enum fields in VhostUserMsg
  vhost: validate untrusted memory.nregions field
  vhost: clear out unused SCM_RIGHTS file descriptors
  vhost: reject invalid log base mmap_offset values
  vhost: fix msg->payload union typo in vhost_user_set_vring_addr()
  vhost: validate virtqueue size
  vhost: check for memory_size + mmap_offset overflow

 lib/librte_vhost/vhost_user.h |  4 +--
 lib/librte_vhost/socket.c     |  8 +++++-
 lib/librte_vhost/vhost_user.c | 57 +++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 64 insertions(+), 5 deletions(-)

-- 
2.14.3

[dpdk-dev] [PATCH 1/8] vhost: add security model documentation to vhost_user.c

[Stefan Hajnoczi]

Input validation is not applied consistently in vhost_user.c.  This
suggests that not everyone has the same security model in mind when
working on the code.

Make the security model explicit so that everyone can understand and
follow the same model when modifying the code.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 1dd1a61b6..a96afbe84 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -2,6 +2,23 @@
  * Copyright(c) 2010-2016 Intel Corporation
  */
 
+/* Security model
+ * --------------
+ * The vhost-user protocol connection is an external interface, so it must be
+ * robust against invalid inputs.
+ *
+ * This is important because the vhost-user master is only one step removed
+ * from the guest.  Malicious guests that have escaped will then launch further
+ * attacks from the vhost-user master.
+ *
+ * Even in deployments where guests are trusted, a bug in the vhost-user master
+ * can still cause invalid messages to be sent.  Such messages must not
+ * compromise the stability of the DPDK application by causing crashes, memory
+ * corruption, or other problematic behavior.
+ *
+ * Do not assume received VhostUserMsg fields contain sensible values!
+ */
+
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
-- 
2.14.3

[dpdk-dev] [PATCH 2/8] vhost: avoid enum fields in VhostUserMsg

[Stefan Hajnoczi]

The VhostUserMsg struct binary representation must match the vhost-user
protocol specification since this struct is read from and written to the
socket.

The VhostUserMsg.request union contains enum fields.  Enum binary
representation is implementation-defined according to the C standard and
it is unportable to make assumptions about the representation:

  6.7.2.2 Enumeration specifiers
  ...
  Each enumerated type shall be compatible with char, a signed integer
  type, or an unsigned integer type. The choice of type is
  implementation-defined, but shall be capable of representing the
  values of all the members of the enumeration.

Additionally, librte_vhost relies on the enum type being unsigned when
validating untrusted inputs:

  if (ret <= 0 || msg.request.master >= VHOST_USER_MAX) {

If msg.request.master is signed then negative values pass this check!

Even if we assume gcc on x86_64 (SysV amd64 ABI) and don't care about
portability, the actual enum constants still affect the final type.  For
example, if we add a negative constant then its type changes to signed
int:

  typedef enum VhostUserRequest {
      ...
      VHOST_USER_INVALID = -1,
  };

This is very fragile and it's unlikely that anyone changing the code
would remember this.  A security hole can be introduced accidentally.

This patch switches VhostUserMsg.request fields to uint32_t to avoid the
portability and potential security issues.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 lib/librte_vhost/vhost_user.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
index d4bd604b9..0fafbe6e0 100644
--- a/lib/librte_vhost/vhost_user.h
+++ b/lib/librte_vhost/vhost_user.h
@@ -81,8 +81,8 @@ typedef struct VhostUserLog {
 
 typedef struct VhostUserMsg {
 	union {
-		VhostUserRequest master;
-		VhostUserSlaveRequest slave;
+		uint32_t master; /* a VhostUserRequest value */
+		uint32_t slave;  /* a VhostUserSlaveRequest value*/
 	} request;
 
 #define VHOST_USER_VERSION_MASK     0x3
-- 
2.14.3

[dpdk-dev] [PATCH 3/8] vhost: validate untrusted memory.nregions field

[Stefan Hajnoczi]

Check if memory.nregions is valid right away.  This eliminates the
possibility of bugs when memory.nregions is used later on in
vhost_user_set_mem_table().

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index a96afbe84..48b493d70 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -662,6 +662,12 @@ vhost_user_set_mem_table(struct virtio_net *dev, struct VhostUserMsg *pmsg)
 	uint32_t i;
 	int fd;
 
+	if (memory.nregions > VHOST_MEMORY_MAX_NREGIONS) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"too many memory regions (%u)\n", memory.nregions);
+		return -1;
+	}
+
 	if (dev->mem && !vhost_memory_changed(&memory, dev->mem)) {
 		RTE_LOG(INFO, VHOST_CONFIG,
 			"(%d) memory regions not changed\n", dev->vid);
-- 
2.14.3

[dpdk-dev] [PATCH 4/8] vhost: clear out unused SCM_RIGHTS file descriptors

[Stefan Hajnoczi]

The number of file descriptors received is not stored by vhost_user.c.
vhost_user_set_mem_table() assumes that memory.nregions matches the
number of file descriptors received, but nothing guarantees this:

  for (i = 0; i < memory.nregions; i++)
      close(pmsg->fds[i]);

Another questionable code snippet is:

  case VHOST_USER_SET_LOG_FD:
      close(msg.fds[0]);

If not enough file descriptors were received then fds[] contains
uninitialized data from the stack (see read_fd_message()).  This might
cause non-vhost file descriptors to be closed if the uninitialized data
happens to match.

Refactoring vhost_user.c to pass around and check the number of file
descriptors everywhere would make the code more complex.  It is simpler
for read_fd_message() to set unused elements in fds[] to -1.  This way
close(-1) is called and no harm is done.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 lib/librte_vhost/socket.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 6e3857e7a..4e3f8abb9 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -96,6 +96,7 @@ read_fd_message(int sockfd, char *buf, int buflen, int *fds, int fd_num)
 	size_t fdsize = fd_num * sizeof(int);
 	char control[CMSG_SPACE(fdsize)];
 	struct cmsghdr *cmsg;
+	int got_fds = 0;
 	int ret;
 
 	memset(&msgh, 0, sizeof(msgh));
@@ -122,11 +123,16 @@ read_fd_message(int sockfd, char *buf, int buflen, int *fds, int fd_num)
 		cmsg = CMSG_NXTHDR(&msgh, cmsg)) {
 		if ((cmsg->cmsg_level == SOL_SOCKET) &&
 			(cmsg->cmsg_type == SCM_RIGHTS)) {
-			memcpy(fds, CMSG_DATA(cmsg), fdsize);
+			got_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
+			memcpy(fds, CMSG_DATA(cmsg), got_fds * sizeof(int));
 			break;
 		}
 	}
 
+	/* Clear out unused file descriptors */
+	while (got_fds < fd_num)
+		fds[got_fds++] = -1;
+
 	return ret;
 }
 
-- 
2.14.3

[dpdk-dev] [PATCH 5/8] vhost: reject invalid log base mmap_offset values

[Stefan Hajnoczi]

If the log base mmap_offset is larger than mmap_size then it points
outside the mmap region.  We must not write to memory outside the mmap
region, so validate mmap_offset in vhost_user_set_log_base().

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 48b493d70..fd47404ce 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1005,6 +1005,15 @@ vhost_user_set_log_base(struct virtio_net *dev, struct VhostUserMsg *msg)
 
 	size = msg->payload.log.mmap_size;
 	off  = msg->payload.log.mmap_offset;
+
+	/* Don't allow mmap_offset to point outside the mmap region */
+	if (off > size) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"log offset %#"PRIx64" exceeds log size %#"PRIx64"\n",
+			off, size);
+		return -1;
+	}
+
 	RTE_LOG(INFO, VHOST_CONFIG,
 		"log mmap size: %"PRId64", offset: %"PRId64"\n",
 		size, off);
-- 
2.14.3

[dpdk-dev] [PATCH 6/8] vhost: fix msg->payload union typo in vhost_user_set_vring_addr()

[Stefan Hajnoczi]

vhost_user_set_vring_addr() uses the msg->payload.addr union member, not
msg->payload.state.  Luckily the offset of the 'index' field is
identical in both structs, so there was never any buggy behavior.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index fd47404ce..3a58d1082 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -516,7 +516,7 @@ vhost_user_set_vring_addr(struct virtio_net **pdev, VhostUserMsg *msg)
 
 	if (vq->enabled && (dev->features &
 				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
-		dev = translate_ring_addresses(dev, msg->payload.state.index);
+		dev = translate_ring_addresses(dev, msg->payload.addr.index);
 		if (!dev)
 			return -1;
 
-- 
2.14.3

[dpdk-dev] [PATCH 7/8] vhost: validate virtqueue size

[Stefan Hajnoczi]

Check the virtqueue size constraints so that invalid values don't cause
bugs later on in the code.  For example, sometimes the virtqueue size is
stored as unsigned int and sometimes as uint16_t, so bad things happen
if it is ever larger than 65535.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 3a58d1082..7d282cb36 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -237,6 +237,17 @@ vhost_user_set_vring_num(struct virtio_net *dev,
 
 	vq->size = msg->payload.state.num;
 
+	/* VIRTIO 1.0, 2.4 Virtqueues says:
+	 *
+	 *   Queue Size value is always a power of 2. The maximum Queue Size
+	 *   value is 32768.
+	 */
+	if ((vq->size & (vq->size - 1)) || vq->size > 32768) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"invalid virtqueue size %u\n", vq->size);
+		return -1;
+	}
+
 	if (dev->dequeue_zero_copy) {
 		vq->nr_zmbuf = 0;
 		vq->last_zmbuf_idx = 0;
-- 
2.14.3

[dpdk-dev] [PATCH 8/8] vhost: check for memory_size + mmap_offset overflow

[Stefan Hajnoczi]

If memory_size + mmap_offset overflows then the memory region is bogus.
Do not use the overflowed mmap_size value for mmap().

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 7d282cb36..cf742a558 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -729,7 +729,17 @@ vhost_user_set_mem_table(struct virtio_net *dev, struct VhostUserMsg *pmsg)
 		reg->fd              = fd;
 
 		mmap_offset = memory.regions[i].mmap_offset;
-		mmap_size   = reg->size + mmap_offset;
+
+		/* Check for memory_size + mmap_offset overflow */
+		if (mmap_offset >= -reg->size) {
+			RTE_LOG(ERR, VHOST_CONFIG,
+				"mmap_offset (%#"PRIx64") and memory_size "
+				"(%#"PRIx64") overflow\n",
+				mmap_offset, reg->size);
+			goto err_mmap;
+		}
+
+		mmap_size = reg->size + mmap_offset;
 
 		/* mmap() without flag of MAP_ANONYMOUS, should be called
 		 * with length argument aligned with hugepagesz at older
-- 
2.14.3

Re: [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements

[Maxime Coquelin]

Hi Stefan,

On 02/05/2018 01:16 PM, Stefan Hajnoczi wrote:
> This patch series addresses missing input validation that I came across when
> reviewing vhost_user.c.
> 
> The first patch explains the security model and the rest fixes places with
> missing checks.
> 
> Now is a good time to discuss the security model if anyone disagrees or has
> questions about what Patch 1 says.

Thanks for the series, I agree validating vhost-user inputs is
necessary.

I'll go through the series, but it will be only for v18.05, as we are
close now to v18.02 release.

Maxime

> Stefan Hajnoczi (8):
>    vhost: add security model documentation to vhost_user.c
>    vhost: avoid enum fields in VhostUserMsg
>    vhost: validate untrusted memory.nregions field
>    vhost: clear out unused SCM_RIGHTS file descriptors
>    vhost: reject invalid log base mmap_offset values
>    vhost: fix msg->payload union typo in vhost_user_set_vring_addr()
>    vhost: validate virtqueue size
>    vhost: check for memory_size + mmap_offset overflow
> 
>   lib/librte_vhost/vhost_user.h |  4 +--
>   lib/librte_vhost/socket.c     |  8 +++++-
>   lib/librte_vhost/vhost_user.c | 57 +++++++++++++++++++++++++++++++++++++++++--
>   3 files changed, 64 insertions(+), 5 deletions(-)
> 

Re: [dpdk-dev] [PATCH 2/8] vhost: avoid enum fields in VhostUserMsg

[Maxime Coquelin]

On 02/05/2018 01:16 PM, Stefan Hajnoczi wrote:
> The VhostUserMsg struct binary representation must match the vhost-user
> protocol specification since this struct is read from and written to the
> socket.
> 
> The VhostUserMsg.request union contains enum fields.  Enum binary
> representation is implementation-defined according to the C standard and
> it is unportable to make assumptions about the representation:
> 
>    6.7.2.2 Enumeration specifiers
>    ...
>    Each enumerated type shall be compatible with char, a signed integer
>    type, or an unsigned integer type. The choice of type is
>    implementation-defined, but shall be capable of representing the
>    values of all the members of the enumeration.
> 
> Additionally, librte_vhost relies on the enum type being unsigned when
> validating untrusted inputs:
> 
>    if (ret <= 0 || msg.request.master >= VHOST_USER_MAX) {
> 
> If msg.request.master is signed then negative values pass this check!
> 
> Even if we assume gcc on x86_64 (SysV amd64 ABI) and don't care about
> portability, the actual enum constants still affect the final type.  For
> example, if we add a negative constant then its type changes to signed
> int:
> 
>    typedef enum VhostUserRequest {
>        ...
>        VHOST_USER_INVALID = -1,
>    };
> 
> This is very fragile and it's unlikely that anyone changing the code
> would remember this.  A security hole can be introduced accidentally.
> 
> This patch switches VhostUserMsg.request fields to uint32_t to avoid the
> portability and potential security issues.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   lib/librte_vhost/vhost_user.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> index d4bd604b9..0fafbe6e0 100644
> --- a/lib/librte_vhost/vhost_user.h
> +++ b/lib/librte_vhost/vhost_user.h
> @@ -81,8 +81,8 @@ typedef struct VhostUserLog {
>   
>   typedef struct VhostUserMsg {
>   	union {
> -		VhostUserRequest master;
> -		VhostUserSlaveRequest slave;
> +		uint32_t master; /* a VhostUserRequest value */
> +		uint32_t slave;  /* a VhostUserSlaveRequest value*/
>   	} request;
>   
>   #define VHOST_USER_VERSION_MASK     0x3
> 

Maybe we could simplify to:

typedef struct VhostUserMsg {
  	uint32_t request; /* a VhostUserRequest or VhostUserSlaveRequest value */
...

Also, it seems QEMU's vhost-user master implementation uses an enum for
the request in its VhostUserMsg struct. Should it be fixed too?

Thanks,
Maxime

Re: [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements

[Maxime Coquelin]

On 02/05/2018 01:16 PM, Stefan Hajnoczi wrote:
> This patch series addresses missing input validation that I came across when
> reviewing vhost_user.c.
> 
> The first patch explains the security model and the rest fixes places with
> missing checks.
> 
> Now is a good time to discuss the security model if anyone disagrees or has
> questions about what Patch 1 says.
> 
> Stefan Hajnoczi (8):
>    vhost: add security model documentation to vhost_user.c
>    vhost: avoid enum fields in VhostUserMsg
>    vhost: validate untrusted memory.nregions field
>    vhost: clear out unused SCM_RIGHTS file descriptors
>    vhost: reject invalid log base mmap_offset values
>    vhost: fix msg->payload union typo in vhost_user_set_vring_addr()
>    vhost: validate virtqueue size
>    vhost: check for memory_size + mmap_offset overflow
> 
>   lib/librte_vhost/vhost_user.h |  4 +--
>   lib/librte_vhost/socket.c     |  8 +++++-
>   lib/librte_vhost/vhost_user.c | 57 +++++++++++++++++++++++++++++++++++++++++--
>   3 files changed, 64 insertions(+), 5 deletions(-)
> 

For the series:

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

For patch 2, my suggestion can be handled separately.

Thanks,
Maxime

Re: [dpdk-dev] [PATCH 1/8] vhost: add security model documentation to vhost_user.c

[Kovacevic, Marko]

> Input validation is not applied consistently in vhost_user.c.  This suggests that
> not everyone has the same security model in mind when working on the
> code.
> 
> Make the security model explicit so that everyone can understand and follow
> the same model when modifying the code.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  lib/librte_vhost/vhost_user.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c

<...>

This is a useful comment but I don't know if it makes sense to include it in the vhost_user.c file. 

Particularly at the top where it looks like a general descriptive comment for the file.

It would probably be better in the vhost-user section of the programmer's guide:

http://dpdk.org/doc/guides/prog_guide/vhost_lib.html

Also, the subject line shouldn't include an underscore.

Marko

Re: [dpdk-dev] [PATCH 1/8] vhost: add security model documentation to vhost_user.c

[Kovacevic, Marko]

<...>

> > This is a useful comment but I don't know if it makes sense to include it in
> the vhost_user.c file.
> >
> > Particularly at the top where it looks like a general descriptive comment for
> the file.
> >
> > It would probably be better in the vhost-user section of the programmer's
> guide:
> >
> > http://dpdk.org/doc/guides/prog_guide/vhost_lib.html
> 
> That is the public API documentation for users of the library.  They don't
> parse VhostUserMsg so I'm not sure the comment would be relevant there.
> 
> > Also, the subject line shouldn't include an underscore.
> 
> Do you mean the "vhost_user.c" filename in the subject line?
> 
> Please explain why this isn't allowed.
> 
> Stefan


You can run this command to check the git log.

./devtools/check-git-log.sh -1

Running git check log on HEAD~1:  34962
======================================
Wrong headline format:
        vhost: add security model documentation to vhost_user.c

Re: [dpdk-dev] [PATCH 1/8] vhost: add security model documentation to vhost_user.c

[Mcnamara, John]

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> Sent: Tuesday, February 6, 2018 2:23 PM
> To: Kovacevic, Marko <marko.kovacevic@intel.com>
> Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>; Yuanhan
> Liu <yliu@fridaylinux.org>; Mcnamara, John <john.mcnamara@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 1/8] vhost: add security model
> documentation to vhost_user.c
> 
> On Tue, Feb 06, 2018 at 01:26:13PM +0000, Kovacevic, Marko wrote:
> > > Input validation is not applied consistently in vhost_user.c.  This
> > > suggests that not everyone has the same security model in mind when
> > > working on the code.
> > >
> > > Make the security model explicit so that everyone can understand and
> > > follow the same model when modifying the code.
> > >
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  lib/librte_vhost/vhost_user.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > >
> > > diff --git a/lib/librte_vhost/vhost_user.c
> > > b/lib/librte_vhost/vhost_user.c
> >
> > <...>
> >
> > This is a useful comment but I don't know if it makes sense to include
> it in the vhost_user.c file.
> >
> > Particularly at the top where it looks like a general descriptive
> comment for the file.
> >
> > It would probably be better in the vhost-user section of the
> programmer's guide:
> >
> > http://dpdk.org/doc/guides/prog_guide/vhost_lib.html
> 
> That is the public API documentation for users of the library.  They don't
> parse VhostUserMsg so I'm not sure the comment would be relevant there.

Hi,

If it is public API documentation then it should probably be in a .h file
in doxygen format.

I'm in favour of the information being added, and thanks for that, just not
in that position in that file.

John

Re: [dpdk-dev] [PATCH 1/8] vhost: add security model documentation to vhost_user.c

[Maxime Coquelin]

Hi John,

On 02/07/2018 05:10 PM, Mcnamara, John wrote:
> 
> 
>> -----Original Message-----
>> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
>> Sent: Tuesday, February 6, 2018 2:23 PM
>> To: Kovacevic, Marko <marko.kovacevic@intel.com>
>> Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>; Yuanhan
>> Liu <yliu@fridaylinux.org>; Mcnamara, John <john.mcnamara@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH 1/8] vhost: add security model
>> documentation to vhost_user.c
>>
>> On Tue, Feb 06, 2018 at 01:26:13PM +0000, Kovacevic, Marko wrote:
>>>> Input validation is not applied consistently in vhost_user.c.  This
>>>> suggests that not everyone has the same security model in mind when
>>>> working on the code.
>>>>
>>>> Make the security model explicit so that everyone can understand and
>>>> follow the same model when modifying the code.
>>>>
>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>>   lib/librte_vhost/vhost_user.c | 17 +++++++++++++++++
>>>>   1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/lib/librte_vhost/vhost_user.c
>>>> b/lib/librte_vhost/vhost_user.c
>>>
>>> <...>
>>>
>>> This is a useful comment but I don't know if it makes sense to include
>> it in the vhost_user.c file.
>>>
>>> Particularly at the top where it looks like a general descriptive
>> comment for the file.
>>>
>>> It would probably be better in the vhost-user section of the
>> programmer's guide:
>>>
>>> http://dpdk.org/doc/guides/prog_guide/vhost_lib.html
>>
>> That is the public API documentation for users of the library.  They don't
>> parse VhostUserMsg so I'm not sure the comment would be relevant there.
> 
> Hi,
> 
> If it is public API documentation then it should probably be in a .h file
> in doxygen format.
> 
> I'm in favour of the information being added, and thanks for that, just not
> in that position in that file.

This is not part of the API but purely vhost-user lib internals,
so I think this is the right place for this comment.

It is more likely to be seen by the developer here than in a separate
file.

Cheers,
Maxime

> John
> 

Re: [dpdk-dev] [PATCH 1/8] vhost: add security model documentation to vhost_user.c

[Mcnamara, John]

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Wednesday, February 7, 2018 4:18 PM
> To: Mcnamara, John <john.mcnamara@intel.com>; Stefan Hajnoczi
> <stefanha@redhat.com>; Kovacevic, Marko <marko.kovacevic@intel.com>
> Cc: dev@dpdk.org; Yuanhan Liu <yliu@fridaylinux.org>
> Subject: Re: [dpdk-dev] [PATCH 1/8] vhost: add security model
> documentation to vhost_user.c
> 
> Hi John,
> 
> On 02/07/2018 05:10 PM, Mcnamara, John wrote:
> >
> >
> >> -----Original Message-----
> >> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> >> Sent: Tuesday, February 6, 2018 2:23 PM
> >> To: Kovacevic, Marko <marko.kovacevic@intel.com>
> >> Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>;
> >> Yuanhan Liu <yliu@fridaylinux.org>; Mcnamara, John
> >> <john.mcnamara@intel.com>
> >> Subject: Re: [dpdk-dev] [PATCH 1/8] vhost: add security model
> >> documentation to vhost_user.c
> >>
> >> On Tue, Feb 06, 2018 at 01:26:13PM +0000, Kovacevic, Marko wrote:
> >>>> Input validation is not applied consistently in vhost_user.c.  This
> >>>> suggests that not everyone has the same security model in mind when
> >>>> working on the code.
> >>>>
> >>>> Make the security model explicit so that everyone can understand
> >>>> and follow the same model when modifying the code.
> >>>>
> >>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>>> ---
> >>>>   lib/librte_vhost/vhost_user.c | 17 +++++++++++++++++
> >>>>   1 file changed, 17 insertions(+)
> >>>>
> >>>> diff --git a/lib/librte_vhost/vhost_user.c
> >>>> b/lib/librte_vhost/vhost_user.c
> >>>
> >>> <...>
> >>>
> >>> This is a useful comment but I don't know if it makes sense to
> >>> include
> >> it in the vhost_user.c file.
> >>>
> >>> Particularly at the top where it looks like a general descriptive
> >> comment for the file.
> >>>
> >>> It would probably be better in the vhost-user section of the
> >> programmer's guide:
> >>>
> >>> http://dpdk.org/doc/guides/prog_guide/vhost_lib.html
> >>
> >> That is the public API documentation for users of the library.  They
> >> don't parse VhostUserMsg so I'm not sure the comment would be relevant
> there.
> >
> > Hi,
> >
> > If it is public API documentation then it should probably be in a .h
> > file in doxygen format.
> >
> > I'm in favour of the information being added, and thanks for that,
> > just not in that position in that file.
> 
> This is not part of the API but purely vhost-user lib internals, so I
> think this is the right place for this comment.
> 
> It is more likely to be seen by the developer here than in a separate
> file.

Ok. In that case:

Acked-by: John McNamara <john.mcnamara@intel.com>

Re: [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements

[Maxime Coquelin]

On 02/05/2018 01:16 PM, Stefan Hajnoczi wrote:
> This patch series addresses missing input validation that I came across when
> reviewing vhost_user.c.
> 
> The first patch explains the security model and the rest fixes places with
> missing checks.
> 
> Now is a good time to discuss the security model if anyone disagrees or has
> questions about what Patch 1 says.
> 
> Stefan Hajnoczi (8):
>    vhost: add security model documentation to vhost_user.c
>    vhost: avoid enum fields in VhostUserMsg
>    vhost: validate untrusted memory.nregions field
>    vhost: clear out unused SCM_RIGHTS file descriptors
>    vhost: reject invalid log base mmap_offset values
>    vhost: fix msg->payload union typo in vhost_user_set_vring_addr()
>    vhost: validate virtqueue size
>    vhost: check for memory_size + mmap_offset overflow
> 
>   lib/librte_vhost/vhost_user.h |  4 +--
>   lib/librte_vhost/socket.c     |  8 +++++-
>   lib/librte_vhost/vhost_user.c | 57 +++++++++++++++++++++++++++++++++++++++++--
>   3 files changed, 64 insertions(+), 5 deletions(-)
> 

Applied to dpdk-next-virtio.

Thanks,
Maxime