* [dpdk-dev] [PATCH 1/8] vhost: add security model documentation to vhost_user.c
2018-02-05 12:16 [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements Stefan Hajnoczi
@ 2018-02-05 12:16 ` Stefan Hajnoczi
2018-02-06 13:26 ` Kovacevic, Marko
2018-02-05 12:16 ` [dpdk-dev] [PATCH 2/8] vhost: avoid enum fields in VhostUserMsg Stefan Hajnoczi
` (9 subsequent siblings)
10 siblings, 1 reply; 18+ messages in thread
From: Stefan Hajnoczi @ 2018-02-05 12:16 UTC (permalink / raw)
To: dev; +Cc: Maxime Coquelin, Yuanhan Liu, 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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [dpdk-dev] [PATCH 1/8] vhost: add security model documentation to vhost_user.c
2018-02-05 12:16 ` [dpdk-dev] [PATCH 1/8] vhost: add security model documentation to vhost_user.c Stefan Hajnoczi
@ 2018-02-06 13:26 ` Kovacevic, Marko
[not found] ` <20180206142235.GB13343@stefanha-x1.localdomain>
0 siblings, 1 reply; 18+ messages in thread
From: Kovacevic, Marko @ 2018-02-06 13:26 UTC (permalink / raw)
To: Stefan Hajnoczi, dev; +Cc: Maxime Coquelin, Yuanhan Liu, Mcnamara, John
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [dpdk-dev] [PATCH 2/8] vhost: avoid enum fields in VhostUserMsg
2018-02-05 12:16 [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements Stefan Hajnoczi
2018-02-05 12:16 ` [dpdk-dev] [PATCH 1/8] vhost: add security model documentation to vhost_user.c Stefan Hajnoczi
@ 2018-02-05 12:16 ` Stefan Hajnoczi
2018-02-06 9:47 ` Maxime Coquelin
2018-02-05 12:16 ` [dpdk-dev] [PATCH 3/8] vhost: validate untrusted memory.nregions field Stefan Hajnoczi
` (8 subsequent siblings)
10 siblings, 1 reply; 18+ messages in thread
From: Stefan Hajnoczi @ 2018-02-05 12:16 UTC (permalink / raw)
To: dev; +Cc: Maxime Coquelin, Yuanhan Liu, 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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [dpdk-dev] [PATCH 2/8] vhost: avoid enum fields in VhostUserMsg
2018-02-05 12:16 ` [dpdk-dev] [PATCH 2/8] vhost: avoid enum fields in VhostUserMsg Stefan Hajnoczi
@ 2018-02-06 9:47 ` Maxime Coquelin
0 siblings, 0 replies; 18+ messages in thread
From: Maxime Coquelin @ 2018-02-06 9:47 UTC (permalink / raw)
To: Stefan Hajnoczi, dev; +Cc: Yuanhan Liu
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [dpdk-dev] [PATCH 3/8] vhost: validate untrusted memory.nregions field
2018-02-05 12:16 [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements Stefan Hajnoczi
2018-02-05 12:16 ` [dpdk-dev] [PATCH 1/8] vhost: add security model documentation to vhost_user.c Stefan Hajnoczi
2018-02-05 12:16 ` [dpdk-dev] [PATCH 2/8] vhost: avoid enum fields in VhostUserMsg Stefan Hajnoczi
@ 2018-02-05 12:16 ` Stefan Hajnoczi
2018-02-05 12:16 ` [dpdk-dev] [PATCH 4/8] vhost: clear out unused SCM_RIGHTS file descriptors Stefan Hajnoczi
` (7 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2018-02-05 12:16 UTC (permalink / raw)
To: dev; +Cc: Maxime Coquelin, Yuanhan Liu, 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
^ permalink raw reply [flat|nested] 18+ messages in thread* [dpdk-dev] [PATCH 4/8] vhost: clear out unused SCM_RIGHTS file descriptors
2018-02-05 12:16 [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements Stefan Hajnoczi
` (2 preceding siblings ...)
2018-02-05 12:16 ` [dpdk-dev] [PATCH 3/8] vhost: validate untrusted memory.nregions field Stefan Hajnoczi
@ 2018-02-05 12:16 ` Stefan Hajnoczi
2018-02-05 12:16 ` [dpdk-dev] [PATCH 5/8] vhost: reject invalid log base mmap_offset values Stefan Hajnoczi
` (6 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2018-02-05 12:16 UTC (permalink / raw)
To: dev; +Cc: Maxime Coquelin, Yuanhan Liu, 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
^ permalink raw reply [flat|nested] 18+ messages in thread* [dpdk-dev] [PATCH 5/8] vhost: reject invalid log base mmap_offset values
2018-02-05 12:16 [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements Stefan Hajnoczi
` (3 preceding siblings ...)
2018-02-05 12:16 ` [dpdk-dev] [PATCH 4/8] vhost: clear out unused SCM_RIGHTS file descriptors Stefan Hajnoczi
@ 2018-02-05 12:16 ` Stefan Hajnoczi
2018-02-05 12:16 ` [dpdk-dev] [PATCH 6/8] vhost: fix msg->payload union typo in vhost_user_set_vring_addr() Stefan Hajnoczi
` (5 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2018-02-05 12:16 UTC (permalink / raw)
To: dev; +Cc: Maxime Coquelin, Yuanhan Liu, 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
^ permalink raw reply [flat|nested] 18+ messages in thread* [dpdk-dev] [PATCH 6/8] vhost: fix msg->payload union typo in vhost_user_set_vring_addr()
2018-02-05 12:16 [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements Stefan Hajnoczi
` (4 preceding siblings ...)
2018-02-05 12:16 ` [dpdk-dev] [PATCH 5/8] vhost: reject invalid log base mmap_offset values Stefan Hajnoczi
@ 2018-02-05 12:16 ` Stefan Hajnoczi
2018-02-05 12:16 ` [dpdk-dev] [PATCH 7/8] vhost: validate virtqueue size Stefan Hajnoczi
` (4 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2018-02-05 12:16 UTC (permalink / raw)
To: dev; +Cc: Maxime Coquelin, Yuanhan Liu, 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
^ permalink raw reply [flat|nested] 18+ messages in thread* [dpdk-dev] [PATCH 7/8] vhost: validate virtqueue size
2018-02-05 12:16 [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements Stefan Hajnoczi
` (5 preceding siblings ...)
2018-02-05 12:16 ` [dpdk-dev] [PATCH 6/8] vhost: fix msg->payload union typo in vhost_user_set_vring_addr() Stefan Hajnoczi
@ 2018-02-05 12:16 ` Stefan Hajnoczi
2018-02-05 12:16 ` [dpdk-dev] [PATCH 8/8] vhost: check for memory_size + mmap_offset overflow Stefan Hajnoczi
` (3 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2018-02-05 12:16 UTC (permalink / raw)
To: dev; +Cc: Maxime Coquelin, Yuanhan Liu, 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
^ permalink raw reply [flat|nested] 18+ messages in thread* [dpdk-dev] [PATCH 8/8] vhost: check for memory_size + mmap_offset overflow
2018-02-05 12:16 [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements Stefan Hajnoczi
` (6 preceding siblings ...)
2018-02-05 12:16 ` [dpdk-dev] [PATCH 7/8] vhost: validate virtqueue size Stefan Hajnoczi
@ 2018-02-05 12:16 ` Stefan Hajnoczi
2018-02-06 9:32 ` [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements Maxime Coquelin
` (2 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2018-02-05 12:16 UTC (permalink / raw)
To: dev; +Cc: Maxime Coquelin, Yuanhan Liu, 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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements
2018-02-05 12:16 [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements Stefan Hajnoczi
` (7 preceding siblings ...)
2018-02-05 12:16 ` [dpdk-dev] [PATCH 8/8] vhost: check for memory_size + mmap_offset overflow Stefan Hajnoczi
@ 2018-02-06 9:32 ` Maxime Coquelin
2018-02-06 10:01 ` Maxime Coquelin
2018-02-19 13:52 ` Maxime Coquelin
10 siblings, 0 replies; 18+ messages in thread
From: Maxime Coquelin @ 2018-02-06 9:32 UTC (permalink / raw)
To: Stefan Hajnoczi, dev; +Cc: Yuanhan Liu
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(-)
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements
2018-02-05 12:16 [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements Stefan Hajnoczi
` (8 preceding siblings ...)
2018-02-06 9:32 ` [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements Maxime Coquelin
@ 2018-02-06 10:01 ` Maxime Coquelin
2018-02-19 13:52 ` Maxime Coquelin
10 siblings, 0 replies; 18+ messages in thread
From: Maxime Coquelin @ 2018-02-06 10:01 UTC (permalink / raw)
To: Stefan Hajnoczi, dev; +Cc: Yuanhan Liu
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements
2018-02-05 12:16 [dpdk-dev] [PATCH 0/8] vhost: input validation enhancements Stefan Hajnoczi
` (9 preceding siblings ...)
2018-02-06 10:01 ` Maxime Coquelin
@ 2018-02-19 13:52 ` Maxime Coquelin
10 siblings, 0 replies; 18+ messages in thread
From: Maxime Coquelin @ 2018-02-19 13:52 UTC (permalink / raw)
To: Stefan Hajnoczi, dev; +Cc: Yuanhan Liu
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
^ permalink raw reply [flat|nested] 18+ messages in thread