linux-kernel - Re: [PATCH RESEND] kvm:x86:Fix error handling in the function kvm_write_wall

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:	Thu, 17 Dec 2015 11:36:35 +0100
From:	Paolo Bonzini <pbonzini@...hat.com>
To:	Nicholas Krause <xerofoify@...il.com>, gleb@...nel.org
Cc:	x86@...nel.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND] kvm:x86:Fix error handling in the function
 kvm_write_wall_clock



On 17/12/2015 03:30, Nicholas Krause wrote:
> This fixes error handling in the function kvm_write_wall_clock
> by checking if any of the calls to kvm_write_guest have failed
> inside this paricutlar function and if so print to the console
> with pr_err that we are unable to write the data to the guest
> system to warn the user of this failure before directly returning
> to the caller of the function kvm_write_wall_check as we cannot
> continue the function to this function after a failed call to
> 
> Signed-off-by: Nicholas Krause <xerofoify@...il.com>

I think I have explained before why this is mostly unnecessary, but the
patch can be saved.  I'll go through the problems again first.

> -	kvm_write_guest(kvm, wall_clock, &version, sizeof(version));
> +	if (kvm_write_guest(kvm, wall_clock, &version, sizeof(version))) {
> +		pr_err("Unable to correctly write data to guest system\n");
> +		return;
> +	}

You've written a message to the log that can be triggered by the guest
(by writing an invalid value to the wall clock MSR).  We don't let the
guest spam the host logs.

>  	/*
>  	 * The guest calculates current wall clock time by adding
> @@ -1168,10 +1171,16 @@ static void kvm_write_wall_clock(struct kvm *kvm, gpa_t wall_clock)
>  	wc.nsec = boot.tv_nsec;
>  	wc.version = version;
>  
> -	kvm_write_guest(kvm, wall_clock, &wc, sizeof(wc));
> +	if (kvm_write_guest(kvm, wall_clock, &wc, sizeof(wc))) {
> +		pr_err("Unable to correctly write data to guest system\n");
> +		return;
> +	}

The other kvm_write_guest will probably also fail but, if it doesn't,
you've left an odd version in the data structure. The guest will loop
forever waiting for the even value.

Plus, same problem with logs.

>  	version++;
> -	kvm_write_guest(kvm, wall_clock, &version, sizeof(version));
> +	if (kvm_write_guest(kvm, wall_clock, &version, sizeof(version))) {
> +		pr_err("Unable to correctly write data to guest system\n");
> +		return;
> +	}
>  }

Same problem with logs, and the return is not useful.

Can you send a patch that only adds a return if the *first*
kvm_write_guest fails?  You can leave aside the others, and not add any
pr_err.

Thanks,

Paolo

>  static uint32_t div_frac(uint32_t dividend, uint32_t divisor)
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/