Skip to contentSkip to conversation

KVM: X86: prevent integer overflows in KVM_MEMORY_ENCRYPT_REG_REGION

PATCH In review Dan Carpenter 3 participants 3 messages May 19, 2018 – May 26, 2018

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 KVM: X86: prevent integer overflows in KVM_MEMORY_ENCRYPT_REG_REGION

This is a fix from reviewing the code, but it looks like it might be able to lead to an Oops. It affects 32bit systems.

The KVM_MEMORY_ENCRYPT_REG_REGION ioctl uses a u64 for range->addr and range->size but the high 32 bits would be truncated away on a 32 bit system. This is harmless but it's also harmless to prevent it.

Then in sev_pin_memory() the "uaddr + ulen" calculation can wrap around. The wrap around can happen on 32 bit or 64 bit systems, but I was only able to figure out a problem for 32 bit systems. We would pick a number which results in "npages" being zero. The sev_pin_memory() would then return ZERO_SIZE_PTR without allocating anything.

I made it illegal to call sev_pin_memory() with "ulen" set to zero. Hopefully, that doesn't cause any problems. I also changed the type of "first" and "last" to long, just for cosmetic reasons. Otherwise on a 64 bit system you're saving "uaddr >> 12" in an int and it truncates the high 20 bits away. The math works in the current code so far as I can see but it's just weird.

---
Again, this is a static checker fix. The most risky parts of this patch are blocking "ulen = 0" and changing the types of "first" and "last". I felt like those changes made the math easier to understand

arch/x86/kvm/svm.c +7 -1
index 220e5a89465a..de21d5c5168b 100644
@@ -1762,7 +1762,10 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
 	unsigned long npages, npinned, size;
 	unsigned long locked, lock_limit;
 	struct page **pages;
-	int first, last;
+	unsigned long first, last;
+
+	if (ulen = 0 || uaddr + ulen < uaddr)
+		return NULL;
 
 	/* Calculate number of pages. */
 	first = (uaddr & PAGE_MASK) >> PAGE_SHIFT;
@@ -6925,6 +6928,9 @@ static int svm_register_enc_region(struct kvm *kvm,
 	if (!sev_guest(kvm))
 		return -ENOTTY;
 
+	if (range->addr > ULONG_MAX || range->size > ULONG_MAX)
+		return -EINVAL;
+
 	region = kzalloc(sizeof(*region), GFP_KERNEL);
 	if (!region)
 		return -ENOMEM;
Reviewed-by on v1 1/1
#1 lore raw

Hi Dan,

Quoted 3 lines · Dan Carpenter

This is a fix from reviewing the code, but it looks like it might be able to lead to an Oops. It affects 32bit systems.

Please note that SEV is not available on 32bit systems.

Quoted 4 lines

The KVM_MEMORY_ENCRYPT_REG_REGION ioctl uses a u64 for range->addr and range->size but the high 32 bits would be truncated away on a 32 bit system. This is harmless but it's also harmless to prevent it.

Quoted 8 lines

Then in sev_pin_memory() the "uaddr + ulen" calculation can wrap around. The wrap around can happen on 32 bit or 64 bit systems, but I was only able to figure out a problem for 32 bit systems. We would pick a number which results in "npages" being zero. The sev_pin_memory() would then return ZERO_SIZE_PTR without allocating anything.

I made it illegal to call sev_pin_memory() with "ulen" set to zero. Hopefully, that doesn't cause any problems.

I think this should be fine.

I also changed the type of

Quoted 5 lines

"first" and "last" to long, just for cosmetic reasons. Otherwise on a 64 bit system you're saving "uaddr >> 12" in an int and it truncates the high 20 bits away. The math works in the current code so far as I can see but it's just weird.

This change looks good. thanks

Quoted 1 line
Quoted 32 lines

---
Again, this is a static checker fix. The most risky parts of this patch are blocking "ulen = 0" and changing the types of "first" and "last". I felt like those changes made the math easier to understand

arch/x86/kvm/svm.c +7 -1
index 220e5a89465a..de21d5c5168b 100644
@@ -1762,7 +1762,10 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
  	unsigned long npages, npinned, size;
  	unsigned long locked, lock_limit;
  	struct page **pages;
-	int first, last;
+	unsigned long first, last;
+
+	if (ulen = 0 || uaddr + ulen < uaddr)
+		return NULL;
 
  	/* Calculate number of pages. */
  	first = (uaddr & PAGE_MASK) >> PAGE_SHIFT;
@@ -6925,6 +6928,9 @@ static int svm_register_enc_region(struct kvm *kvm,
  	if (!sev_guest(kvm))
  		return -ENOTTY;
 
+	if (range->addr > ULONG_MAX || range->size > ULONG_MAX)
+		return -EINVAL;
+
  	region = kzalloc(sizeof(*region), GFP_KERNEL);
  	if (!region)
  		return -ENOMEM;
#2 lore raw

2018-05-21 10:52-0500, Brijesh Singh:

Quoted 6 lines · Dan Carpenter
Quoted 3 lines

This is a fix from reviewing the code, but it looks like it might be able to lead to an Oops. It affects 32bit systems.

Please note that SEV is not available on 32bit systems.

Added this note and queued, thanks.