Re: [PATCH 25/82] KVM: SVM: Refactor intentional wrap-around calculation

[Sean Christopherson]

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
> 
> Notably, 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[2], unsigned[3],
> or pointer[4] 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
> wrap-around sanitizers in the future.

IIUC, the plan is to get UBSAN to detect unexpected overflow, at which point an
explicit annotation will be needed to avoid false positives.  If that's correct,
can you put something like that in these changelogs?  Nothing in the changelog
actually says _why_ open coded wrap-around checks will be problematic.

> Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1]
> Link: https://github.com/KSPP/linux/issues/26 [2]
> Link: https://github.com/KSPP/linux/issues/27 [3]
> Link: https://github.com/KSPP/linux/issues/344 [4]
> Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Cc: x86@xxxxxxxxxx
> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
>  arch/x86/kvm/svm/sev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index f760106c31f8..12a6a2b1ac81 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -400,16 +400,17 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
>  	unsigned long locked, lock_limit;
>  	struct page **pages;
>  	unsigned long first, last;
> +	unsigned long sum;

Similar to Marc's comments, I would much prefer to call this uaddr_last.

>  	int ret;
>  
>  	lockdep_assert_held(&kvm->lock);
>  
> -	if (ulen == 0 || uaddr + ulen < uaddr)
> +	if (ulen == 0 || check_add_overflow(uaddr, ulen, &sum))
>  		return ERR_PTR(-EINVAL);
>  
>  	/* Calculate number of pages. */
>  	first = (uaddr & PAGE_MASK) >> PAGE_SHIFT;
> -	last = ((uaddr + ulen - 1) & PAGE_MASK) >> PAGE_SHIFT;
> +	last = ((sum - 1) & PAGE_MASK) >> PAGE_SHIFT;
>  	npages = (last - first + 1);
>  
>  	locked = sev->pages_locked + npages;
> -- 
> 2.34.1
>