Received-SPF: pass (google.com: domain of linux-kernel+bounces-37329-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 24 Jan 2024 08:25:18 -0800
In-Reply-To: <20240123002814.1396804-23-keescook@chromium.org>
Precedence: bulk
Mime-Version: 1.0
References: <20240122235208.work.748-kees@kernel.org> <20240123002814.1396804-23-keescook@chromium.org>
Message-ID: <ZbE57k4gCYZb9h0H@google.com>
Subject: Re: [PATCH 23/82] KVM: Refactor intentional wrap-around calculation
From: Sean Christopherson <seanjc@google.com>
To: Kees Cook <keescook@chromium.org>
Cc: linux-hardening@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>, 
	kvm@vger.kernel.org, "Gustavo A. R. Silva" <gustavoars@kernel.org>, 
	Bill Wendling <morbo@google.com>, Justin Stitt <justinstitt@google.com>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"

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
>