[PATCH kvmtool 0/4] Fix some undefined behaviour

[PATCH kvmtool 0/4] Fix some undefined behaviour

[Andre Przywara]

Hi,

triggered by some integer overflow issues, discovered through Alex' "set
RAM base address" series, I enabled "-fsanitize=undefined
-fsanitize=address" in the Makefile, and enjoyed the fireworks.
This series is cheekily just picking the lowest hanging fruits:
Some needlessly unaligned accesses in the virtio-mmio code, and signed
shifts in the x86 CPUID code.
There are more issues, but they take more time fixing.

Please have a look!

Cheers,
Andre

Andre Przywara (4):
  virtio/mmio: avoid unaligned accesses
  virtio/mmio: access header members normally
  virtio/mmio: remove unneeded virtio_mmio_hdr members
  x86/cpuid: fix undefined behaviour

 include/kvm/virtio-mmio.h | 12 ++----------
 virtio/mmio.c             | 19 +++++++++++++++----
 x86/cpuid.c               |  6 +++---
 3 files changed, 20 insertions(+), 17 deletions(-)

-- 
2.25.1

[PATCH kvmtool 1/4] virtio/mmio: avoid unaligned accesses

[Andre Przywara]

The virtio-mmio code is using unaligned accesses, to its struct
virtio_mmio, as revealed by -fsanitize=undefined.
A closer inspection reveals that this is due to a misplaced u8 member
in struct virtio_mmio, and it inheriting the "packed" attribute from
struct virtio_mmio_hdr.
The simplest fix for the issue is to just move the "u8 irq" member to
the end, so that even with the "packed" attribute in effect, the other
members stay all naturally aligned.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/virtio-mmio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/kvm/virtio-mmio.h b/include/kvm/virtio-mmio.h
index 6bc50bd1..13dcccb6 100644
--- a/include/kvm/virtio-mmio.h
+++ b/include/kvm/virtio-mmio.h
@@ -45,10 +45,10 @@ struct virtio_mmio {
 	u32			addr;
 	void			*dev;
 	struct kvm		*kvm;
-	u8			irq;
 	struct virtio_mmio_hdr	hdr;
 	struct device_header	dev_hdr;
 	struct virtio_mmio_ioevent_param ioeventfds[VIRTIO_MMIO_MAX_VQ];
+	u8			irq;
 };
 
 int virtio_mmio_signal_vq(struct kvm *kvm, struct virtio_device *vdev, u32 vq);
-- 
2.25.1

[PATCH kvmtool 2/4] virtio/mmio: access header members normally

[Andre Przywara]

The handlers for accessing the virtio-mmio header tried to be very
clever, by modelling the internal data structure to look exactly like
the protocol header, so that address offsets can "reused".

This requires using a packed structure, which creates other problems,
and seems to be totally unnecessary in this case.

Replace the offset-based access hacks to the structure with proper
compiler visible accesses, to avoid unaligned accesses and make the code
more robust.

This fixes UBSAN complaints about unaligned accesses.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/virtio-mmio.h |  2 +-
 virtio/mmio.c             | 19 +++++++++++++++----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/include/kvm/virtio-mmio.h b/include/kvm/virtio-mmio.h
index 13dcccb6..aa4cab3c 100644
--- a/include/kvm/virtio-mmio.h
+++ b/include/kvm/virtio-mmio.h
@@ -39,7 +39,7 @@ struct virtio_mmio_hdr {
 	u32	interrupt_ack;
 	u32	reserved_5[2];
 	u32	status;
-} __attribute__((packed));
+};
 
 struct virtio_mmio {
 	u32			addr;
diff --git a/virtio/mmio.c b/virtio/mmio.c
index 3782d55a..c9ad8ee7 100644
--- a/virtio/mmio.c
+++ b/virtio/mmio.c
@@ -135,12 +135,22 @@ static void virtio_mmio_config_in(struct kvm_cpu *vcpu,
 
 	switch (addr) {
 	case VIRTIO_MMIO_MAGIC_VALUE:
+		memcpy(data, &vmmio->hdr.magic, sizeof(vmmio->hdr.magic));
+		break;
 	case VIRTIO_MMIO_VERSION:
+		ioport__write32(data, vmmio->hdr.version);
+		break;
 	case VIRTIO_MMIO_DEVICE_ID:
+		ioport__write32(data, vmmio->hdr.device_id);
+		break;
 	case VIRTIO_MMIO_VENDOR_ID:
+		ioport__write32(data, vmmio->hdr.vendor_id);
+		break;
 	case VIRTIO_MMIO_STATUS:
+		ioport__write32(data, vmmio->hdr.status);
+		break;
 	case VIRTIO_MMIO_INTERRUPT_STATUS:
-		ioport__write32(data, *(u32 *)(((void *)&vmmio->hdr) + addr));
+		ioport__write32(data, vmmio->hdr.interrupt_state);
 		break;
 	case VIRTIO_MMIO_DEVICE_FEATURES:
 		if (vmmio->hdr.host_features_sel == 0)
@@ -174,9 +184,10 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu,
 
 	switch (addr) {
 	case VIRTIO_MMIO_DEVICE_FEATURES_SEL:
+		vmmio->hdr.host_features_sel = ioport__read32(data);
+		break;
 	case VIRTIO_MMIO_DRIVER_FEATURES_SEL:
-		val = ioport__read32(data);
-		*(u32 *)(((void *)&vmmio->hdr) + addr) = val;
+		vmmio->hdr.guest_features_sel = ioport__read32(data);
 		break;
 	case VIRTIO_MMIO_QUEUE_SEL:
 		val = ioport__read32(data);
@@ -185,7 +196,7 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu,
 				val, vq_count);
 			break;
 		}
-		*(u32 *)(((void *)&vmmio->hdr) + addr) = val;
+		vmmio->hdr.queue_sel = val;
 		break;
 	case VIRTIO_MMIO_STATUS:
 		vmmio->hdr.status = ioport__read32(data);
-- 
2.25.1

[PATCH kvmtool 3/4] virtio/mmio: remove unneeded virtio_mmio_hdr members

[Andre Przywara]

So far "struct virtio_mmio_hdr" was modelled exactly like the virtio
MMIO protocol header, including reserved fields and members unused by
kvmtool.
Since we now no longer need to stay byte-for-byte compatible, drop those
members to clean up the code.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/virtio-mmio.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/include/kvm/virtio-mmio.h b/include/kvm/virtio-mmio.h
index aa4cab3c..84848eee 100644
--- a/include/kvm/virtio-mmio.h
+++ b/include/kvm/virtio-mmio.h
@@ -22,22 +22,14 @@ struct virtio_mmio_hdr {
 	u32	vendor_id;
 	u32	host_features;
 	u32	host_features_sel;
-	u32	reserved_1[2];
 	u32	guest_features;
 	u32	guest_features_sel;
 	u32	guest_page_size;
-	u32	reserved_2;
 	u32	queue_sel;
 	u32	queue_num_max;
 	u32	queue_num;
 	u32	queue_align;
-	u32	queue_pfn;
-	u32	reserved_3[3];
-	u32	queue_notify;
-	u32	reserved_4[3];
 	u32	interrupt_state;
-	u32	interrupt_ack;
-	u32	reserved_5[2];
 	u32	status;
 };
 
-- 
2.25.1

[PATCH kvmtool 4/4] x86/cpuid: fix undefined behaviour

[Andre Przywara]

Shifting signed values is rarely a good idea, especially if the result
ends up setting the most significant bit. UBSAN warns about two
occasions in the CPUID filter code:
===========================
x86/cpuid.c:23:25: runtime error: left shift of 255 by 24 places cannot be represented in type 'int'
x86/cpuid.c:27:22: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
===========================

Fix those warnings by making sure we only deal with unsigned values.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 x86/cpuid.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/x86/cpuid.c b/x86/cpuid.c
index f4347a84..1ae681ce 100644
--- a/x86/cpuid.c
+++ b/x86/cpuid.c
@@ -8,7 +8,7 @@
 
 #define	MAX_KVM_CPUID_ENTRIES		100
 
-static void filter_cpuid(struct kvm_cpuid2 *kvm_cpuid, int cpu_id)
+static void filter_cpuid(struct kvm_cpuid2 *kvm_cpuid, unsigned int cpu_id)
 {
 	unsigned int i;
 
@@ -20,11 +20,11 @@ static void filter_cpuid(struct kvm_cpuid2 *kvm_cpuid, int cpu_id)
 
 		switch (entry->function) {
 		case 1:
-			entry->ebx &= ~(0xff << 24);
+			entry->ebx &= ~(0xffU << 24);
 			entry->ebx |= cpu_id << 24;
 			/* Set X86_FEATURE_HYPERVISOR */
 			if (entry->index == 0)
-				entry->ecx |= (1 << 31);
+				entry->ecx |= (1U << 31);
 			break;
 		case 6:
 			/* Clear X86_FEATURE_EPB */
-- 
2.25.1

Re: [PATCH kvmtool 2/4] virtio/mmio: access header members normally

[Will Deacon]

On Wed, Jun 01, 2022 at 05:51:36PM +0100, Andre Przywara wrote:
> The handlers for accessing the virtio-mmio header tried to be very
> clever, by modelling the internal data structure to look exactly like
> the protocol header, so that address offsets can "reused".
> 
> This requires using a packed structure, which creates other problems,
> and seems to be totally unnecessary in this case.
> 
> Replace the offset-based access hacks to the structure with proper
> compiler visible accesses, to avoid unaligned accesses and make the code
> more robust.
> 
> This fixes UBSAN complaints about unaligned accesses.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  include/kvm/virtio-mmio.h |  2 +-
>  virtio/mmio.c             | 19 +++++++++++++++----
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/include/kvm/virtio-mmio.h b/include/kvm/virtio-mmio.h
> index 13dcccb6..aa4cab3c 100644
> --- a/include/kvm/virtio-mmio.h
> +++ b/include/kvm/virtio-mmio.h
> @@ -39,7 +39,7 @@ struct virtio_mmio_hdr {
>  	u32	interrupt_ack;
>  	u32	reserved_5[2];
>  	u32	status;
> -} __attribute__((packed));
> +};

Does this mean that the previous patch is no longer required?

>  
>  struct virtio_mmio {
>  	u32			addr;
> diff --git a/virtio/mmio.c b/virtio/mmio.c
> index 3782d55a..c9ad8ee7 100644
> --- a/virtio/mmio.c
> +++ b/virtio/mmio.c
> @@ -135,12 +135,22 @@ static void virtio_mmio_config_in(struct kvm_cpu *vcpu,
>  
>  	switch (addr) {
>  	case VIRTIO_MMIO_MAGIC_VALUE:
> +		memcpy(data, &vmmio->hdr.magic, sizeof(vmmio->hdr.magic));

Hmm, this is a semantic change as we used to treat the magic as a u32 by
passing it to ioport__write32(), which would in turn do the swab for
big-endian machines.

I don't think we should be using raw memcpy() here.

Will

Re: [PATCH kvmtool 2/4] virtio/mmio: access header members normally

[Andre Przywara]

On Tue, 7 Jun 2022 11:36:58 +0100
Will Deacon <will@kernel.org> wrote:

Hi Will,

> On Wed, Jun 01, 2022 at 05:51:36PM +0100, Andre Przywara wrote:
> > The handlers for accessing the virtio-mmio header tried to be very
> > clever, by modelling the internal data structure to look exactly like
> > the protocol header, so that address offsets can "reused".
> > 
> > This requires using a packed structure, which creates other problems,
> > and seems to be totally unnecessary in this case.
> > 
> > Replace the offset-based access hacks to the structure with proper
> > compiler visible accesses, to avoid unaligned accesses and make the code
> > more robust.
> > 
> > This fixes UBSAN complaints about unaligned accesses.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  include/kvm/virtio-mmio.h |  2 +-
> >  virtio/mmio.c             | 19 +++++++++++++++----
> >  2 files changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/kvm/virtio-mmio.h b/include/kvm/virtio-mmio.h
> > index 13dcccb6..aa4cab3c 100644
> > --- a/include/kvm/virtio-mmio.h
> > +++ b/include/kvm/virtio-mmio.h
> > @@ -39,7 +39,7 @@ struct virtio_mmio_hdr {
> >  	u32	interrupt_ack;
> >  	u32	reserved_5[2];
> >  	u32	status;
> > -} __attribute__((packed));
> > +};  
> 
> Does this mean that the previous patch is no longer required?

To some degree patch 1/4 is the quick fix. But I think ordering
struct members in an efficient way is never a bad idea, so that patch
still has some use.

> >  struct virtio_mmio {
> >  	u32			addr;
> > diff --git a/virtio/mmio.c b/virtio/mmio.c
> > index 3782d55a..c9ad8ee7 100644
> > --- a/virtio/mmio.c
> > +++ b/virtio/mmio.c
> > @@ -135,12 +135,22 @@ static void virtio_mmio_config_in(struct kvm_cpu *vcpu,
> >  
> >  	switch (addr) {
> >  	case VIRTIO_MMIO_MAGIC_VALUE:
> > +		memcpy(data, &vmmio->hdr.magic, sizeof(vmmio->hdr.magic));  
> 
> Hmm, this is a semantic change as we used to treat the magic as a u32 by
> passing it to ioport__write32(), which would in turn do the swab for
> big-endian machines.

Ah, it's big endian testing time again (is it already that time of the
year?)

> 
> I don't think we should be using raw memcpy() here.

I will check, thanks for having a look!

Cheers,
Andre