Hi,
Here are 6 patches that harden console, net and 9p drivers against
various malicious host input as well as close a bounds check bypass
in the split virtio ring.
Changes since previous version:
* Added Christian's R-B to 3/6
* Added a speculation fix per Michael's comment on the cover letter
* CC'ing lkml
Alexander Shishkin (3):
virtio console: Harden control message handling
virtio_net: Guard against buffer length overflow in
xdp_linearize_page()
virtio_ring: Prevent bounds check bypass on descriptor index
Andi Kleen (3):
virtio console: Harden multiport against invalid host input
virtio console: Harden port adding
virtio 9p: Fix an overflow
drivers/char/virtio_console.c | 19 ++++++++++++-------
drivers/net/virtio_net.c | 4 +++-
drivers/virtio/virtio_ring.c | 3 +++
net/9p/trans_virtio.c | 2 +-
4 files changed, 19 insertions(+), 9 deletions(-)
--
2.39.0
The descriptor index in virtqueue_get_buf_ctx_split() comes from the
device/VMM.a Use array_index_nospec() to prevent the CPU from speculating
beyond the descriptor array bounds and providing a primitive for building
a side channel.
Signed-off-by: Alexander Shishkin <[email protected]>
---
drivers/virtio/virtio_ring.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 2e7689bb933b..c42d070ab68d 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -9,6 +9,7 @@
#include <linux/device.h>
#include <linux/slab.h>
#include <linux/module.h>
+#include <linux/nospec.h>
#include <linux/hrtimer.h>
#include <linux/dma-mapping.h>
#include <linux/kmsan.h>
@@ -819,6 +820,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
BAD_RING(vq, "id %u out of range\n", i);
return NULL;
}
+
+ i = array_index_nospec(i, vq->split.vring.num);
if (unlikely(!vq->split.desc_state[i].data)) {
BAD_RING(vq, "id %u is not a head!\n", i);
return NULL;
--
2.39.0
From: Andi Kleen <[email protected]>
It's possible for the host to set the multiport flag, but pass in
0 multiports, which results in:
BUG: KASAN: slab-out-of-bounds in init_vqs+0x244/0x6c0 drivers/char/virtio_console.c:1878
Write of size 8 at addr ffff888001cc24a0 by task swapper/1
CPU: 0 PID: 1 Comm: swapper Not tainted 5.15.0-rc1-140273-gaab0bb9fbaa1-dirty #588
Call Trace:
init_vqs+0x244/0x6c0 drivers/char/virtio_console.c:1878
virtcons_probe+0x1a3/0x5b0 drivers/char/virtio_console.c:2042
virtio_dev_probe+0x2b9/0x500 drivers/virtio/virtio.c:263
call_driver_probe drivers/base/dd.c:515
really_probe+0x1c9/0x5b0 drivers/base/dd.c:601
really_probe_debug drivers/base/dd.c:694
__driver_probe_device+0x10d/0x1f0 drivers/base/dd.c:754
driver_probe_device+0x68/0x150 drivers/base/dd.c:786
__driver_attach+0xca/0x200 drivers/base/dd.c:1145
bus_for_each_dev+0x108/0x190 drivers/base/bus.c:301
driver_attach+0x30/0x40 drivers/base/dd.c:1162
bus_add_driver+0x325/0x3c0 drivers/base/bus.c:618
driver_register+0xf3/0x1d0 drivers/base/driver.c:171
...
Add a suitable sanity check.
Signed-off-by: Andi Kleen <[email protected]>
Signed-off-by: Alexander Shishkin <[email protected]>
Cc: Amit Shah <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/char/virtio_console.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 6a821118d553..f4fd5fe7cd3a 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1843,6 +1843,9 @@ static int init_vqs(struct ports_device *portdev)
int err;
nr_ports = portdev->max_nr_ports;
+ if (use_multiport(portdev) && nr_ports < 1)
+ return -EINVAL;
+
nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
vqs = kmalloc_array(nr_queues, sizeof(struct virtqueue *), GFP_KERNEL);
--
2.39.0
From: Andi Kleen <[email protected]>
The ADD_PORT operation reads and sanity checks the port id multiple
times from the untrusted host. This is not safe because a malicious
host could change it between reads.
Read the port id only once and cache it for subsequent uses.
Signed-off-by: Andi Kleen <[email protected]>
Signed-off-by: Alexander Shishkin <[email protected]>
Cc: Amit Shah <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/char/virtio_console.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index f4fd5fe7cd3a..6599c2956ba4 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1563,10 +1563,13 @@ static void handle_control_message(struct virtio_device *vdev,
struct port *port;
size_t name_size;
int err;
+ unsigned id;
cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
- port = find_port_by_id(portdev, virtio32_to_cpu(vdev, cpkt->id));
+ /* Make sure the host cannot change id under us */
+ id = virtio32_to_cpu(vdev, READ_ONCE(cpkt->id));
+ port = find_port_by_id(portdev, id);
if (!port &&
cpkt->event != cpu_to_virtio16(vdev, VIRTIO_CONSOLE_PORT_ADD)) {
/* No valid header at start of buffer. Drop it. */
@@ -1583,15 +1586,14 @@ static void handle_control_message(struct virtio_device *vdev,
send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
break;
}
- if (virtio32_to_cpu(vdev, cpkt->id) >=
- portdev->max_nr_ports) {
+ if (id >= portdev->max_nr_ports) {
dev_warn(&portdev->vdev->dev,
"Request for adding port with "
"out-of-bound id %u, max. supported id: %u\n",
cpkt->id, portdev->max_nr_ports - 1);
break;
}
- add_port(portdev, virtio32_to_cpu(vdev, cpkt->id));
+ add_port(portdev, id);
break;
case VIRTIO_CONSOLE_PORT_REMOVE:
unplug_port(port);
--
2.39.0
In handle_control_message(), we look at the ->event field twice, which
gives a malicious VMM a window in which to switch it from PORT_ADD to
PORT_REMOVE, triggering a null dereference further down the line:
RIP: 0010:spin_lock_irq ./include/linux/spinlock.h:388
RIP: 0010:unplug_port+0x9/0x150 drivers/char/virtio_console.c:1512
Call Trace:
handle_control_message+0x108/0x2c0 drivers/char/virtio_console.c:1600
elfcorehdr_read+0x40/0x40 ??:?
process_one_work+0x1b4/0x310 kernel/workqueue.c:2297
worker_thread+0x5c/0x3a0 kernel/workqueue.c:2444
kthread+0x120/0x140 kernel/kthread.c:319
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
Read the event code once instead, basing all following decisions on the
same value.
Signed-off-by: Alexander Shishkin <[email protected]>
Cc: Amit Shah <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/char/virtio_console.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 6599c2956ba4..62f69f949cb7 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1563,22 +1563,22 @@ static void handle_control_message(struct virtio_device *vdev,
struct port *port;
size_t name_size;
int err;
- unsigned id;
+ unsigned id, event;
cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
- /* Make sure the host cannot change id under us */
+ /* Make sure the host cannot change id or event under us */
id = virtio32_to_cpu(vdev, READ_ONCE(cpkt->id));
+ event = virtio16_to_cpu(vdev, cpkt->event);
port = find_port_by_id(portdev, id);
- if (!port &&
- cpkt->event != cpu_to_virtio16(vdev, VIRTIO_CONSOLE_PORT_ADD)) {
+ if (!port && event != VIRTIO_CONSOLE_PORT_ADD) {
/* No valid header at start of buffer. Drop it. */
dev_dbg(&portdev->vdev->dev,
"Invalid index %u in control packet\n", cpkt->id);
return;
}
- switch (virtio16_to_cpu(vdev, cpkt->event)) {
+ switch (event) {
case VIRTIO_CONSOLE_PORT_ADD:
if (port) {
dev_dbg(&portdev->vdev->dev,
--
2.39.0
On Thu, Jan 19, 2023 at 03:57:19PM +0200, Alexander Shishkin wrote:
> In handle_control_message(), we look at the ->event field twice, which
> gives a malicious VMM a window in which to switch it from PORT_ADD to
> PORT_REMOVE, triggering a null dereference further down the line:
How is the other VMM have full control over the full message here?
Shouldn't this all have been copied into our local memory if we are
going to be poking around in it? Like I mentioned in my other review,
copy it all once and then parse it. Don't try to mess with individual
fields one at a time otherwise that way lies madness...
thanks,
greg k-h
On Thu, Jan 19, 2023 at 03:57:15PM +0200, Alexander Shishkin wrote:
> Hi,
>
> Here are 6 patches that harden console, net and 9p drivers against
> various malicious host input as well as close a bounds check bypass
> in the split virtio ring.
Hardening against buggy devices is one thing,
Hardening against malicious devices is another.
Which is this?
If really malicious, aren't there any spectre considerations here?
I am for example surprised not to find anything addressing
spectre v1 nor any uses of array_index_nospec here.
> Changes since previous version:
> * Added Christian's R-B to 3/6
> * Added a speculation fix per Michael's comment on the cover letter
> * CC'ing lkml
>
> Alexander Shishkin (3):
> virtio console: Harden control message handling
> virtio_net: Guard against buffer length overflow in
> xdp_linearize_page()
> virtio_ring: Prevent bounds check bypass on descriptor index
>
> Andi Kleen (3):
> virtio console: Harden multiport against invalid host input
> virtio console: Harden port adding
> virtio 9p: Fix an overflow
>
> drivers/char/virtio_console.c | 19 ++++++++++++-------
> drivers/net/virtio_net.c | 4 +++-
> drivers/virtio/virtio_ring.c | 3 +++
> net/9p/trans_virtio.c | 2 +-
> 4 files changed, 19 insertions(+), 9 deletions(-)
>
> --
> 2.39.0
"Michael S. Tsirkin" <[email protected]> writes:
> On Thu, Jan 19, 2023 at 03:57:15PM +0200, Alexander Shishkin wrote:
>> Hi,
>>
>> Here are 6 patches that harden console, net and 9p drivers against
>> various malicious host input as well as close a bounds check bypass
>> in the split virtio ring.
>
> Hardening against buggy devices is one thing,
> Hardening against malicious devices is another.
> Which is this?
Well, the big difference is the intent, but buggy input is buggy input,
they've got that in common and we're trying to deal with it here.
The motivation for this patchset is protecting against malicious
devices.
> If really malicious, aren't there any spectre considerations here?
> I am for example surprised not to find anything addressing
> spectre v1 nor any uses of array_index_nospec here.
That's strange, patch 6/6 is exactly that. There's probably more coming
in the future as the analysis and audit progress.
Regards,
--
Alex
On Thu, Jan 19, 2023 at 03:57:17PM +0200, Alexander Shishkin wrote:
> From: Andi Kleen <[email protected]>
>
> The ADD_PORT operation reads and sanity checks the port id multiple
> times from the untrusted host. This is not safe because a malicious
> host could change it between reads.
>
> Read the port id only once and cache it for subsequent uses.
>
> Signed-off-by: Andi Kleen <[email protected]>
> Signed-off-by: Alexander Shishkin <[email protected]>
> Cc: Amit Shah <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
I suspect anyone worried about this kind of thing already uses a bounce
buffer. No? The patch itself makes the code more readable, except maybe
for the READ_ONCE thing.
> ---
> drivers/char/virtio_console.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index f4fd5fe7cd3a..6599c2956ba4 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1563,10 +1563,13 @@ static void handle_control_message(struct virtio_device *vdev,
> struct port *port;
> size_t name_size;
> int err;
> + unsigned id;
>
> cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
>
> - port = find_port_by_id(portdev, virtio32_to_cpu(vdev, cpkt->id));
> + /* Make sure the host cannot change id under us */
> + id = virtio32_to_cpu(vdev, READ_ONCE(cpkt->id));
> + port = find_port_by_id(portdev, id);
> if (!port &&
> cpkt->event != cpu_to_virtio16(vdev, VIRTIO_CONSOLE_PORT_ADD)) {
> /* No valid header at start of buffer. Drop it. */
> @@ -1583,15 +1586,14 @@ static void handle_control_message(struct virtio_device *vdev,
> send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
> break;
> }
> - if (virtio32_to_cpu(vdev, cpkt->id) >=
> - portdev->max_nr_ports) {
> + if (id >= portdev->max_nr_ports) {
> dev_warn(&portdev->vdev->dev,
> "Request for adding port with "
> "out-of-bound id %u, max. supported id: %u\n",
> cpkt->id, portdev->max_nr_ports - 1);
> break;
> }
> - add_port(portdev, virtio32_to_cpu(vdev, cpkt->id));
> + add_port(portdev, id);
> break;
> case VIRTIO_CONSOLE_PORT_REMOVE:
> unplug_port(port);
> --
> 2.39.0
On Thu, Jan 19, 2023 at 03:57:16PM +0200, Alexander Shishkin wrote:
> From: Andi Kleen <[email protected]>
>
> It's possible for the host to set the multiport flag, but pass in
> 0 multiports, which results in:
>
> BUG: KASAN: slab-out-of-bounds in init_vqs+0x244/0x6c0 drivers/char/virtio_console.c:1878
> Write of size 8 at addr ffff888001cc24a0 by task swapper/1
>
> CPU: 0 PID: 1 Comm: swapper Not tainted 5.15.0-rc1-140273-gaab0bb9fbaa1-dirty #588
> Call Trace:
> init_vqs+0x244/0x6c0 drivers/char/virtio_console.c:1878
> virtcons_probe+0x1a3/0x5b0 drivers/char/virtio_console.c:2042
> virtio_dev_probe+0x2b9/0x500 drivers/virtio/virtio.c:263
> call_driver_probe drivers/base/dd.c:515
> really_probe+0x1c9/0x5b0 drivers/base/dd.c:601
> really_probe_debug drivers/base/dd.c:694
> __driver_probe_device+0x10d/0x1f0 drivers/base/dd.c:754
> driver_probe_device+0x68/0x150 drivers/base/dd.c:786
> __driver_attach+0xca/0x200 drivers/base/dd.c:1145
> bus_for_each_dev+0x108/0x190 drivers/base/bus.c:301
> driver_attach+0x30/0x40 drivers/base/dd.c:1162
> bus_add_driver+0x325/0x3c0 drivers/base/bus.c:618
> driver_register+0xf3/0x1d0 drivers/base/driver.c:171
> ...
>
> Add a suitable sanity check.
>
> Signed-off-by: Andi Kleen <[email protected]>
> Signed-off-by: Alexander Shishkin <[email protected]>
> Cc: Amit Shah <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> ---
> drivers/char/virtio_console.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 6a821118d553..f4fd5fe7cd3a 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1843,6 +1843,9 @@ static int init_vqs(struct ports_device *portdev)
> int err;
>
> nr_ports = portdev->max_nr_ports;
> + if (use_multiport(portdev) && nr_ports < 1)
> + return -EINVAL;
> +
> nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
>
> vqs = kmalloc_array(nr_queues, sizeof(struct virtqueue *), GFP_KERNEL);
Weird. Don't we already check for that?
/* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
if (!is_rproc_serial(vdev) &&
virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
struct virtio_console_config, max_nr_ports,
&portdev->max_nr_ports) == 0) {
if (portdev->max_nr_ports == 0 ||
portdev->max_nr_ports > VIRTCONS_MAX_PORTS) {
dev_err(&vdev->dev,
"Invalidate max_nr_ports %d",
portdev->max_nr_ports);
err = -EINVAL;
goto free;
}
multiport = true;
}
> --
> 2.39.0
On Thu, Jan 19, 2023 at 04:22:09PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 19, 2023 at 03:57:19PM +0200, Alexander Shishkin wrote:
> > In handle_control_message(), we look at the ->event field twice, which
> > gives a malicious VMM a window in which to switch it from PORT_ADD to
> > PORT_REMOVE, triggering a null dereference further down the line:
>
> How is the other VMM have full control over the full message here?
> Shouldn't this all have been copied into our local memory if we are
> going to be poking around in it? Like I mentioned in my other review,
> copy it all once and then parse it. Don't try to mess with individual
> fields one at a time otherwise that way lies madness...
>
> thanks,
>
> greg k-h
I agree and in fact, it is *already* copied since with malicious
device we generally use a bounce buffer.
Having said that, the patch is actually a cleanup, e.g. it's clearer
to byte-swap only once.
Just don't oversell it as a security thing.
--
MST
On Thu, Jan 19, 2023 at 03:57:21PM +0200, Alexander Shishkin wrote:
> The descriptor index in virtqueue_get_buf_ctx_split() comes from the
> device/VMM.a Use array_index_nospec() to prevent the CPU from speculating
> beyond the descriptor array bounds and providing a primitive for building
> a side channel.
>
> Signed-off-by: Alexander Shishkin <[email protected]>
> ---
> drivers/virtio/virtio_ring.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 2e7689bb933b..c42d070ab68d 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -9,6 +9,7 @@
> #include <linux/device.h>
> #include <linux/slab.h>
> #include <linux/module.h>
> +#include <linux/nospec.h>
> #include <linux/hrtimer.h>
> #include <linux/dma-mapping.h>
> #include <linux/kmsan.h>
> @@ -819,6 +820,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
> BAD_RING(vq, "id %u out of range\n", i);
> return NULL;
> }
> +
> + i = array_index_nospec(i, vq->split.vring.num);
I suspect plain
i &= split.vring.num - 1
is more efficient.
We know num is a power of two but compiler doesn't.
And pls add a comment explaining what's going on.
> if (unlikely(!vq->split.desc_state[i].data)) {
> BAD_RING(vq, "id %u is not a head!\n", i);
> return NULL;
> --
> 2.39.0
On Fri, Jan 20, 2023 at 02:32:09PM +0200, Alexander Shishkin wrote:
> "Michael S. Tsirkin" <[email protected]> writes:
>
> > On Thu, Jan 19, 2023 at 03:57:15PM +0200, Alexander Shishkin wrote:
> >> Hi,
> >>
> >> Here are 6 patches that harden console, net and 9p drivers against
> >> various malicious host input as well as close a bounds check bypass
> >> in the split virtio ring.
> >
> > Hardening against buggy devices is one thing,
> > Hardening against malicious devices is another.
> > Which is this?
>
> Well, the big difference is the intent, but buggy input is buggy input,
> they've got that in common and we're trying to deal with it here.
>
> The motivation for this patchset is protecting against malicious
> devices.
>
> > If really malicious, aren't there any spectre considerations here?
> > I am for example surprised not to find anything addressing
> > spectre v1 nor any uses of array_index_nospec here.
>
> That's strange, patch 6/6 is exactly that. There's probably more coming
> in the future as the analysis and audit progress.
>
> Regards,
Oh I see, didn't get it for some reason. Pulled it from lore now.
> --
> Alex
"Michael S. Tsirkin" <[email protected]> writes:
> Weird. Don't we already check for that?
>
> /* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
> if (!is_rproc_serial(vdev) &&
> virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
> struct virtio_console_config, max_nr_ports,
> &portdev->max_nr_ports) == 0) {
> if (portdev->max_nr_ports == 0 ||
> portdev->max_nr_ports > VIRTCONS_MAX_PORTS) {
> dev_err(&vdev->dev,
> "Invalidate max_nr_ports %d",
> portdev->max_nr_ports);
> err = -EINVAL;
> goto free;
> }
> multiport = true;
> }
Yes, I missed this earlier. I'll drop this patch.
Thanks,
--
Alex
"Michael S. Tsirkin" <[email protected]> writes:
> On Thu, Jan 19, 2023 at 04:22:09PM +0100, Greg Kroah-Hartman wrote:
>> On Thu, Jan 19, 2023 at 03:57:19PM +0200, Alexander Shishkin wrote:
>> > In handle_control_message(), we look at the ->event field twice, which
>> > gives a malicious VMM a window in which to switch it from PORT_ADD to
>> > PORT_REMOVE, triggering a null dereference further down the line:
>>
>> How is the other VMM have full control over the full message here?
>> Shouldn't this all have been copied into our local memory if we are
>> going to be poking around in it? Like I mentioned in my other review,
>> copy it all once and then parse it. Don't try to mess with individual
>> fields one at a time otherwise that way lies madness...
>>
>> thanks,
>>
>> greg k-h
>
> I agree and in fact, it is *already* copied since with malicious
> device we generally use a bounce buffer.
Right, but the code should probably be able to handle bad input on its
own, or what do you think?
> Having said that, the patch is actually a cleanup, e.g. it's clearer
> to byte-swap only once.
> Just don't oversell it as a security thing.
Well, security was the original motivation, so that's what it said in
the commit message. But we settled on [0] yesterday with Greg, which
would replace this patch and 2/6.
[0] https://lore.kernel.org/all/[email protected]/
Regards,
--
Alex
On Fri, Jan 20, 2023 at 06:41:27PM +0200, Alexander Shishkin wrote:
> "Michael S. Tsirkin" <[email protected]> writes:
>
> > On Thu, Jan 19, 2023 at 04:22:09PM +0100, Greg Kroah-Hartman wrote:
> >> On Thu, Jan 19, 2023 at 03:57:19PM +0200, Alexander Shishkin wrote:
> >> > In handle_control_message(), we look at the ->event field twice, which
> >> > gives a malicious VMM a window in which to switch it from PORT_ADD to
> >> > PORT_REMOVE, triggering a null dereference further down the line:
> >>
> >> How is the other VMM have full control over the full message here?
> >> Shouldn't this all have been copied into our local memory if we are
> >> going to be poking around in it? Like I mentioned in my other review,
> >> copy it all once and then parse it. Don't try to mess with individual
> >> fields one at a time otherwise that way lies madness...
> >>
> >> thanks,
> >>
> >> greg k-h
> >
> > I agree and in fact, it is *already* copied since with malicious
> > device we generally use a bounce buffer.
>
> Right, but the code should probably be able to handle bad input on its
> own, or what do you think?
Basically I think it's ok to look at the same field twice unless
it's mapped as dma coherent. Is that what you are asking about?
> > Having said that, the patch is actually a cleanup, e.g. it's clearer
> > to byte-swap only once.
> > Just don't oversell it as a security thing.
>
> Well, security was the original motivation, so that's what it said in
> the commit message. But we settled on [0] yesterday with Greg, which
> would replace this patch and 2/6.
>
> [0] https://lore.kernel.org/all/[email protected]/
>
> Regards,
At this point I will drop this series and pls post new series
with just the stuff you want included. Include acks if patches
are unchanged.
Thanks!
> --
> Alex
"Michael S. Tsirkin" <[email protected]> writes:
> At this point I will drop this series and pls post new series
> with just the stuff you want included. Include acks if patches
> are unchanged.
Will do, thanks for the review!
Regards,
--
Alex