diff mbox series

[23/82] KVM: Refactor intentional wrap-around calculation

Message ID 20240123002814.1396804-23-keescook@chromium.org (mailing list archive)
State Changes Requested
Headers show
Series overflow: Refactor open-coded arithmetic wrap-around | expand

Commit Message

Kees Cook Jan. 23, 2024, 12:26 a.m. UTC
In an effort to separate intentional arithmetic wrap-around from
unexpected wrap-around, we need to refactor places that depend on this
kind of math. One of the most common code patterns of this is:

	VAR + value < VAR

Notable, this is considered "undefined behavior" for signed and pointer
types, which the kernel works around by using the -fno-strict-overflow
option in the build[1] (which used to just be -fwrapv). Regardless, we
want to get the kernel source to the position where we can meaningfully
instrument arithmetic wrap-around conditions and catch them when they
are unexpected, regardless of whether they are signed, unsigned, or
pointer types.

Refactor open-coded unsigned wrap-around addition test to use
check_add_overflow(), retaining the result for later usage (which removes
the redundant open-coded addition). This paves the way to enabling the
unsigned wrap-around sanitizer[2] in the future.

Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1]
Link: https://github.com/KSPP/linux/issues/27 [2]
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: kvm@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 virt/kvm/coalesced_mmio.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Sean Christopherson Jan. 24, 2024, 4:25 p.m. UTC | #1
On Mon, Jan 22, 2024, Kees Cook wrote:
> In an effort to separate intentional arithmetic wrap-around from
> unexpected wrap-around, we need to refactor places that depend on this
> kind of math. One of the most common code patterns of this is:
> 
> 	VAR + value < VAR
> 
> Notable, this is considered "undefined behavior" for signed and pointer
> types, which the kernel works around by using the -fno-strict-overflow
> option in the build[1] (which used to just be -fwrapv). Regardless, we
> want to get the kernel source to the position where we can meaningfully
> instrument arithmetic wrap-around conditions and catch them when they
> are unexpected, regardless of whether they are signed, unsigned, or
> pointer types.
> 
> Refactor open-coded unsigned wrap-around addition test to use
> check_add_overflow(), retaining the result for later usage (which removes
> the redundant open-coded addition). This paves the way to enabling the
> unsigned wrap-around sanitizer[2] in the future.
> 
> Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1]
> Link: https://github.com/KSPP/linux/issues/27 [2]
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: kvm@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  virt/kvm/coalesced_mmio.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> index 1b90acb6e3fe..0a3b706fbf4c 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -25,17 +25,19 @@ static inline struct kvm_coalesced_mmio_dev *to_mmio(struct kvm_io_device *dev)
>  static int coalesced_mmio_in_range(struct kvm_coalesced_mmio_dev *dev,
>  				   gpa_t addr, int len)
>  {
> +	gpa_t sum;

s/sum/end?

Also, given that your're fixing a gpa_t, which is a u64, presumably that means
that this code in __kvm_set_memory_region() also needs to be fixed:

	if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr)
		return -EINVAL; 

and for that one I'd really like to avoid an ignored output parameter (KVM
converts the incoming mem->memory_size to pages, so the "sum" is never used
directly).

Would it make sense to add an API that feeds a dummy "sum" value?  I assume UBSAN
won't fire on the usage of the known good value, i.e. using the output parameter
isn't necessary for functional correctness.  Having an API that does just the
check would trim down the size of many of these patches and avoid having to come
up with names for the local variables.  And IMO, the existing code is a wee bit
more intuitive, it'd be nice to give developers the flexibility to choose which
flavor yields the "best" code on a case-by-case basis.

> +
>  	/* is it in a batchable area ?
>  	 * (addr,len) is fully included in
>  	 * (zone->addr, zone->size)
>  	 */
>  	if (len < 0)
>  		return 0;
> -	if (addr + len < addr)
> +	if (check_add_overflow(addr, len, &sum))
>  		return 0;
>  	if (addr < dev->zone.addr)
>  		return 0;
> -	if (addr + len > dev->zone.addr + dev->zone.size)
> +	if (sum > dev->zone.addr + dev->zone.size)
>  		return 0;
>  	return 1;
>  }
> -- 
> 2.34.1
>
diff mbox series

Patch

diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
index 1b90acb6e3fe..0a3b706fbf4c 100644
--- a/virt/kvm/coalesced_mmio.c
+++ b/virt/kvm/coalesced_mmio.c
@@ -25,17 +25,19 @@  static inline struct kvm_coalesced_mmio_dev *to_mmio(struct kvm_io_device *dev)
 static int coalesced_mmio_in_range(struct kvm_coalesced_mmio_dev *dev,
 				   gpa_t addr, int len)
 {
+	gpa_t sum;
+
 	/* is it in a batchable area ?
 	 * (addr,len) is fully included in
 	 * (zone->addr, zone->size)
 	 */
 	if (len < 0)
 		return 0;
-	if (addr + len < addr)
+	if (check_add_overflow(addr, len, &sum))
 		return 0;
 	if (addr < dev->zone.addr)
 		return 0;
-	if (addr + len > dev->zone.addr + dev->zone.size)
+	if (sum > dev->zone.addr + dev->zone.size)
 		return 0;
 	return 1;
 }