Hi,
On Fri, Mar 04, 2022 at 01:10:47AM +0200, Martin Radev wrote:
> The handling of VIRTIO_PCI_O_CONFIG is prone to buffer access overflows.
> This patch sanitizes this operation by using the newly added virtio op
> get_config_size. Any access which goes beyond the config structure's
> size is prevented and a failure is returned.
>
> Additionally, PCI accesses which span more than a single byte are prevented
> and a warning is printed because the implementation does not currently
> support the behavior correctly.
>
> Signed-off-by: Martin Radev <martin.b.radev@xxxxxxxxx>
> ---
> include/kvm/virtio-9p.h | 1 +
> include/kvm/virtio.h | 1 +
> virtio/9p.c | 25 ++++++++++++++++++++-----
> virtio/balloon.c | 8 ++++++++
> virtio/blk.c | 8 ++++++++
> virtio/console.c | 8 ++++++++
> virtio/mmio.c | 24 ++++++++++++++++++++----
> virtio/net.c | 8 ++++++++
> virtio/pci.c | 38 ++++++++++++++++++++++++++++++++++++++
> virtio/rng.c | 6 ++++++
> virtio/scsi.c | 8 ++++++++
> virtio/vsock.c | 8 ++++++++
> 12 files changed, 134 insertions(+), 9 deletions(-)
>
> diff --git a/include/kvm/virtio-9p.h b/include/kvm/virtio-9p.h
> index 3ea7698..77c5062 100644
> --- a/include/kvm/virtio-9p.h
> +++ b/include/kvm/virtio-9p.h
> @@ -44,6 +44,7 @@ struct p9_dev {
> struct virtio_device vdev;
> struct rb_root fids;
>
> + size_t config_size;
> struct virtio_9p_config *config;
> u32 features;
>
> diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h
> index 3a311f5..3880e74 100644
> --- a/include/kvm/virtio.h
> +++ b/include/kvm/virtio.h
> @@ -184,6 +184,7 @@ struct virtio_device {
>
> struct virtio_ops {
> u8 *(*get_config)(struct kvm *kvm, void *dev);
> + size_t (*get_config_size)(struct kvm *kvm, void *dev);
> u32 (*get_host_features)(struct kvm *kvm, void *dev);
> void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features);
> int (*get_vq_count)(struct kvm *kvm, void *dev);
> diff --git a/virtio/9p.c b/virtio/9p.c
> index b78f2b3..6074f3a 100644
> --- a/virtio/9p.c
> +++ b/virtio/9p.c
> @@ -1375,6 +1375,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
> return ((u8 *)(p9dev->config));
> }
>
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> + struct p9_dev *p9dev = dev;
> +
> + return p9dev->config_size;
> +}
> +
> static u32 get_host_features(struct kvm *kvm, void *dev)
> {
> return 1 << VIRTIO_9P_MOUNT_TAG;
> @@ -1469,6 +1476,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>
> struct virtio_ops p9_dev_virtio_ops = {
> .get_config = get_config,
> + .get_config_size = get_config_size,
> .get_host_features = get_host_features,
> .set_guest_features = set_guest_features,
> .init_vq = init_vq,
> @@ -1568,7 +1576,9 @@ virtio_dev_init(virtio_9p__init);
> int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name)
> {
> struct p9_dev *p9dev;
> - int err = 0;
> + size_t tag_name_length;
I think it would be better to name the variable tag_len, the same name as
the corresponding field in struct virtio_9p_config. As a bonus, it's also
shorter. But this is personal preference in the end, so I leave it up to
you to decide which works better.
> + size_t config_size;
> + int err;
>
> p9dev = calloc(1, sizeof(*p9dev));
> if (!p9dev)
> @@ -1577,29 +1587,34 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name)
> if (!tag_name)
> tag_name = VIRTIO_9P_DEFAULT_TAG;
>
> - p9dev->config = calloc(1, sizeof(*p9dev->config) + strlen(tag_name) + 1);
> + tag_name_length = strlen(tag_name);
> + /* The tag_name zero byte is intentionally excluded */
If this is indeed a bug (the comment from virtio_9p_config seems to suggest
it is, but I couldn't find the 9p spec), the bug is that the config size is
computed incorrectly, which is a different bug than a guest being able to
write outside of the config region for the device. As such, it should be
fixed in a separate patch.
> + config_size = sizeof(*p9dev->config) + tag_name_length;
> +
> + p9dev->config = calloc(1, config_size);
> if (p9dev->config == NULL) {
> err = -ENOMEM;
> goto free_p9dev;
> }
> + p9dev->config_size = config_size;
>
> strncpy(p9dev->root_dir, root, sizeof(p9dev->root_dir));
> p9dev->root_dir[sizeof(p9dev->root_dir)-1] = '\x00';
>
> - p9dev->config->tag_len = strlen(tag_name);
> + p9dev->config->tag_len = tag_name_length;
> if (p9dev->config->tag_len > MAX_TAG_LEN) {
> err = -EINVAL;
> goto free_p9dev_config;
> }
>
> - memcpy(&p9dev->config->tag, tag_name, strlen(tag_name));
> + memcpy(&p9dev->config->tag, tag_name, tag_name_length);
>
> list_add(&p9dev->list, &devs);
>
> if (compat_id == -1)
> compat_id = virtio_compat_add_message("virtio-9p", "CONFIG_NET_9P_VIRTIO");
>
> - return err;
> + return 0;
>
> free_p9dev_config:
> free(p9dev->config);
> diff --git a/virtio/balloon.c b/virtio/balloon.c
> index 8e8803f..5bcd6ab 100644
> --- a/virtio/balloon.c
> +++ b/virtio/balloon.c
> @@ -181,6 +181,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
> return ((u8 *)(&bdev->config));
> }
>
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> + struct bln_dev *bdev = dev;
> +
> + return sizeof(bdev->config);
> +}
> +
> static u32 get_host_features(struct kvm *kvm, void *dev)
> {
> return 1 << VIRTIO_BALLOON_F_STATS_VQ;
> @@ -251,6 +258,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>
> struct virtio_ops bln_dev_virtio_ops = {
> .get_config = get_config,
> + .get_config_size = get_config_size,
> .get_host_features = get_host_features,
> .set_guest_features = set_guest_features,
> .init_vq = init_vq,
> diff --git a/virtio/blk.c b/virtio/blk.c
> index 4d02d10..af71c0c 100644
> --- a/virtio/blk.c
> +++ b/virtio/blk.c
> @@ -146,6 +146,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
> return ((u8 *)(&bdev->blk_config));
> }
>
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> + struct blk_dev *bdev = dev;
> +
> + return sizeof(bdev->blk_config);
> +}
> +
> static u32 get_host_features(struct kvm *kvm, void *dev)
> {
> struct blk_dev *bdev = dev;
> @@ -291,6 +298,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>
> static struct virtio_ops blk_dev_virtio_ops = {
> .get_config = get_config,
> + .get_config_size = get_config_size,
> .get_host_features = get_host_features,
> .set_guest_features = set_guest_features,
> .get_vq_count = get_vq_count,
> diff --git a/virtio/console.c b/virtio/console.c
> index e0b98df..dae6034 100644
> --- a/virtio/console.c
> +++ b/virtio/console.c
> @@ -121,6 +121,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
> return ((u8 *)(&cdev->config));
> }
>
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> + struct con_dev *cdev = dev;
> +
> + return sizeof(cdev->config);
> +}
> +
> static u32 get_host_features(struct kvm *kvm, void *dev)
> {
> return 0;
> @@ -216,6 +223,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>
> static struct virtio_ops con_dev_virtio_ops = {
> .get_config = get_config,
> + .get_config_size = get_config_size,
> .get_host_features = get_host_features,
> .set_guest_features = set_guest_features,
> .get_vq_count = get_vq_count,
> diff --git a/virtio/mmio.c b/virtio/mmio.c
> index 875a288..0094856 100644
> --- a/virtio/mmio.c
> +++ b/virtio/mmio.c
> @@ -103,15 +103,31 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu,
> u8 is_write, struct virtio_device *vdev)
> {
> struct virtio_mmio *vmmio = vdev->virtio;
> + u8 *config_aperture;
> + size_t config_aperture_size;
These could be shortened to config and config_size, to better match the
callback names (get_config, respectively get_config_size) and make the code
easier to understand.
> u32 i;
>
> + /* Check for wrap-around. */
> + if (addr + len < addr) {
No need for this, you can move patch #5 before this one, and that should
take care of any wrap arounds.
> + WARN_ONCE(1, "addr (%llu) + length (%u) wraps-around.\n", addr, len);
> + return;
> + }
> +
> + config_aperture = vdev->ops->get_config(vmmio->kvm, vmmio->dev);
> + config_aperture_size = vdev->ops->get_config_size(vmmio->kvm, vmmio->dev);
> +
> + /* Prevent invalid accesses which go beyond the config */
> + if (config_aperture_size < addr + len) {
> + WARN_ONCE(1, "Offset (%llu) Length (%u) goes beyond config size (%zu).\n",
> + addr, len, config_aperture_size);
> + return;
> + }
> +
> for (i = 0; i < len; i++) {
> if (is_write)
> - vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] =
> - *(u8 *)data + i;
> + config_aperture[addr + i] = *(u8 *)data + i;
> else
> - data[i] = vdev->ops->get_config(vmmio->kvm,
> - vmmio->dev)[addr + i];
> + data[i] = config_aperture[addr + i];
> }
> }
>
> diff --git a/virtio/net.c b/virtio/net.c
> index 1ee3c19..ec5dc1f 100644
> --- a/virtio/net.c
> +++ b/virtio/net.c
> @@ -480,6 +480,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
> return ((u8 *)(&ndev->config));
> }
>
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> + struct net_dev *ndev = dev;
> +
> + return sizeof(ndev->config);
> +}
> +
> static u32 get_host_features(struct kvm *kvm, void *dev)
> {
> u32 features;
> @@ -757,6 +764,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>
> static struct virtio_ops net_dev_virtio_ops = {
> .get_config = get_config,
> + .get_config_size = get_config_size,
> .get_host_features = get_host_features,
> .set_guest_features = set_guest_features,
> .get_vq_count = get_vq_count,
> diff --git a/virtio/pci.c b/virtio/pci.c
> index 2777d1c..0b5cccd 100644
> --- a/virtio/pci.c
> +++ b/virtio/pci.c
> @@ -136,7 +136,25 @@ static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device *
> return true;
> } else if (type == VIRTIO_PCI_O_CONFIG) {
> u8 cfg;
> + size_t config_size;
>
> + config_size = vdev->ops->get_config_size(kvm, vpci->dev);
> + if (size <= 0) {
> + WARN_ONCE(1, "Size (%d) is non-positive\n", size);
> + return false;
> + }
This is a different bug. The kvm_run.mmio struct report length as an u32
and the type is preserved until virtio_pci__data_{in,out} are called from
virtio_pci__mmio_callback(). The correct fix is to change the size
parameter from virtio_pci__data_{in,out} and
virtio_pci__specific_data_{in,out} to an u32 in a separate patch.
[1] https://elixir.bootlin.com/linux/v5.17-rc8/source/Documentation/virt/kvm/api.rst#L5726
Thanks,
Alex
> + if (config_offset + (u32)size > config_size) {
> + /* Access goes beyond the config size, so return failure. */
> + WARN_ONCE(1, "Config access offset (%u) is beyond config size (%zu)\n",
> + config_offset, config_size);
> + return false;
> + }
> +
> + /* TODO: Handle access lengths beyond one byte */
> + if (size != 1) {
> + WARN_ONCE(1, "Size (%d) not supported\n", size);
> + return false;
> + }
> cfg = vdev->ops->get_config(kvm, vpci->dev)[config_offset];
> ioport__write8(data, cfg);
> return true;
> @@ -276,6 +294,26 @@ static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device
>
> return true;
> } else if (type == VIRTIO_PCI_O_CONFIG) {
> + size_t config_size;
> +
> + if (size <= 0) {
> + WARN_ONCE(1, "Size (%d) is non-positive\n", size);
> + return false;
> + }
> +
> + config_size = vdev->ops->get_config_size(kvm, vpci->dev);
> + if (config_offset + (u32)size > config_size) {
> + /* Access goes beyond the config size, so return failure. */
> + WARN_ONCE(1, "Config access offset (%u) is beyond config size (%zu)\n",
> + config_offset, config_size);
> + return false;
> + }
> +
> + /* TODO: Handle access lengths beyond one byte */
> + if (size != 1) {
> + WARN_ONCE(1, "Size (%d) not supported\n", size);
> + return false;
> + }
> vdev->ops->get_config(kvm, vpci->dev)[config_offset] = *(u8 *)data;
>
> return true;
> diff --git a/virtio/rng.c b/virtio/rng.c
> index 78eaa64..c7835a0 100644
> --- a/virtio/rng.c
> +++ b/virtio/rng.c
> @@ -47,6 +47,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
> return 0;
> }
>
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> + return 0;
> +}
> +
> static u32 get_host_features(struct kvm *kvm, void *dev)
> {
> /* Unused */
> @@ -149,6 +154,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>
> static struct virtio_ops rng_dev_virtio_ops = {
> .get_config = get_config,
> + .get_config_size = get_config_size,
> .get_host_features = get_host_features,
> .set_guest_features = set_guest_features,
> .init_vq = init_vq,
> diff --git a/virtio/scsi.c b/virtio/scsi.c
> index 16a86cb..8f1c348 100644
> --- a/virtio/scsi.c
> +++ b/virtio/scsi.c
> @@ -38,6 +38,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
> return ((u8 *)(&sdev->config));
> }
>
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> + struct scsi_dev *sdev = dev;
> +
> + return sizeof(sdev->config);
> +}
> +
> static u32 get_host_features(struct kvm *kvm, void *dev)
> {
> return 1UL << VIRTIO_RING_F_EVENT_IDX |
> @@ -176,6 +183,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>
> static struct virtio_ops scsi_dev_virtio_ops = {
> .get_config = get_config,
> + .get_config_size = get_config_size,
> .get_host_features = get_host_features,
> .set_guest_features = set_guest_features,
> .init_vq = init_vq,
> diff --git a/virtio/vsock.c b/virtio/vsock.c
> index 5b99838..34397b6 100644
> --- a/virtio/vsock.c
> +++ b/virtio/vsock.c
> @@ -41,6 +41,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
> return ((u8 *)(&vdev->config));
> }
>
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> + struct vsock_dev *vdev = dev;
> +
> + return sizeof(vdev->config);
> +}
> +
> static u32 get_host_features(struct kvm *kvm, void *dev)
> {
> return 1UL << VIRTIO_RING_F_EVENT_IDX
> @@ -204,6 +211,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>
> static struct virtio_ops vsock_dev_virtio_ops = {
> .get_config = get_config,
> + .get_config_size = get_config_size,
> .get_host_features = get_host_features,
> .set_guest_features = set_guest_features,
> .init_vq = init_vq,
> --
> 2.25.1
>
