| Message ID | 38a2b7e584b2d76a6b8d116e9e291edb3b0ef50d.1291987020.git.amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
> Check if the guest really sent any items in the out_vq before using > them. Similarly, check if there is a buffer to send data in before > writing. Can this actually happen? If so why/how? Why does it need a special case in this device? If this is guest triggerable then calling abort() is wrong. Paul
On (Fri) Dec 10 2010 [13:59:50], Paul Brook wrote: > > Check if the guest really sent any items in the out_vq before using > > them. Similarly, check if there is a buffer to send data in before > > writing. > > Can this actually happen? If so why/how? > Why does it need a special case in this device? A malicious guest (ie, a guest with the virtio_console module suitably modified) could send in buffers with the 'input' bit set instead of output as expected or vice-versa. > If this is guest triggerable then calling abort() is wrong. It's either a guest bug or a malicious guest. What action is recommended? Amit
> On (Fri) Dec 10 2010 [13:59:50], Paul Brook wrote: > > > Check if the guest really sent any items in the out_vq before using > > > them. Similarly, check if there is a buffer to send data in before > > > writing. > > > > Can this actually happen? If so why/how? > > Why does it need a special case in this device? > > A malicious guest (ie, a guest with the virtio_console module suitably > modified) could send in buffers with the 'input' bit set instead of > output as expected or vice-versa. So what? Who cares if they get it wrong? It's entirely unclear whether this is actually an error. If a request has zero size then we just transfer zero bytes, exactly as requested. Even if you accept this should be a diagnosable error, I suspect your patch is still insufficient. I don't see any code to check that input queue requests have zero output segments, nor do I see anything to handle zero-length segments. > > If this is guest triggerable then calling abort() is wrong. > > It's either a guest bug or a malicious guest. What action is > recommended? Killing the whole VM in response to a malformed request to a device is clearly a bug in that device. You should report an error to the guest in the normal manner. IIRC virtio lacks any consistent error reporting mechanisms, and the usual response when asked to do something impossible is to reset the device. Paul
On (Fri) Dec 10 2010 [15:17:58], Paul Brook wrote: > > On (Fri) Dec 10 2010 [13:59:50], Paul Brook wrote: > > > > Check if the guest really sent any items in the out_vq before using > > > > them. Similarly, check if there is a buffer to send data in before > > > > writing. > > > > > > Can this actually happen? If so why/how? > > > Why does it need a special case in this device? > > > > A malicious guest (ie, a guest with the virtio_console module suitably > > modified) could send in buffers with the 'input' bit set instead of > > output as expected or vice-versa. > > So what? Who cares if they get it wrong? Just let the error_report() be there and continue as if nothing happened? > It's entirely unclear whether this is actually an error. If a request has zero > size then we just transfer zero bytes, exactly as requested. > > Even if you accept this should be a diagnosable error, I suspect your patch is > still insufficient. I don't see any code to check that input queue requests > have zero output segments, nor do I see anything to handle zero-length > segments. virtio actually supports sending both, input as well as output types of buffers in one go. > > > If this is guest triggerable then calling abort() is wrong. > > > > It's either a guest bug or a malicious guest. What action is > > recommended? > > Killing the whole VM in response to a malformed request to a device is clearly > a bug in that device. You should report an error to the guest in the normal > manner. IIRC virtio lacks any consistent error reporting mechanisms, and the > usual response when asked to do something impossible is to reset the device. OK, agreed. Amit
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 3bbd915..3a3032f 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -102,6 +102,11 @@ static size_t write_to_port(VirtIOSerialPort *port, break; } + if (elem.in_num < 1) { + error_report("No buffer to send data in?"); + abort(); + } + len = iov_from_buf(elem.in_sg, elem.in_num, buf + offset, size - offset); offset += len; @@ -127,6 +132,11 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq, while (virtqueue_pop(vq, &elem)) { unsigned int i; + if (elem.out_num < 1) { + error_report("No data sent by guest?"); + abort(); + } + if (discard) { goto next; } @@ -169,6 +179,11 @@ static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len) return 0; } + if (elem.in_num < 1) { + error_report("No buffer to send control data in?"); + abort(); + } + cpkt = (struct virtio_console_control *)buf; stl_p(&cpkt->id, port->id); memcpy(elem.in_sg[0].iov_base, buf, len); @@ -386,6 +401,10 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq) while (virtqueue_pop(vq, &elem)) { size_t cur_len, copied; + if (elem.out_num < 1) { + error_report("No data sent in control packet"); + abort(); + } cur_len = iov_size(elem.out_sg, elem.out_num); /* * Allocate a new buf only if we didn't have one previously or
Check if the guest really sent any items in the out_vq before using them. Similarly, check if there is a buffer to send data in before writing. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- hw/virtio-serial-bus.c | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-)