linux-kernel - Re: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in update_permission

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Jun 2018 18:05:01 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Matthias Kaehlcke <mka@...omium.org>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H . Peter Anvin" <hpa@...or.com>
Cc:     x86@...nel.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Joe Perches <joe@...ches.com>
Subject: Re: [PATCH v2] kvm: x86: mmu: Add cast to negated bitmasks in
 update_permission_bitmask()

On 19/06/2018 21:25, Matthias Kaehlcke wrote:
> update_permission_bitmask() negates u8 bitmask values and assigns them
> to variables of type u8. Since the MSB is set in the bitmask values the
> compiler expands the negated values to int, which then is assigned to
> an u8 variable. Cast the negated value back to u8.
> 
> This fixes several warnings like this when building with clang:
> 
> arch/x86/kvm/mmu.c:4266:39: error: implicit conversion from 'int' to 'u8'
>   (aka 'unsigned char') changes value from -205 to 51 [-Werror,
>   -Wconstant-conversion]
>     u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
>        ~~                               ^~
> 
> (gcc also raises a warning (see https://godbolt.org/g/6JWfWk), however it
> doesn't seem to be universally enabled)
> 
> Suggested-by: Nick Desaulniers <ndesaulniers@...gle.com>
> Signed-off-by: Matthias Kaehlcke <mka@...omium.org>
> ---
> Changes in v2:
> - negate the bitmask at initialization and rename variables to not_X

The patch is not that bad, but I'd like to get confirmation that other
maintainers are applying fixes like this.  Honestly I'm not really
impressed by most new clang warnings, these days.

Paolo

>  arch/x86/kvm/mmu.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index d634f0332c0f..ad0a8c35f27b 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4258,8 +4258,9 @@ static void update_permission_bitmask(struct kvm_vcpu *vcpu,
>  {
>  	unsigned byte;
>  
> -	const u8 x = BYTE_MASK(ACC_EXEC_MASK);
> -	const u8 w = BYTE_MASK(ACC_WRITE_MASK);
> +	const u8 not_x = (u8)~BYTE_MASK(ACC_EXEC_MASK);
> +	const u8 not_w = (u8)~BYTE_MASK(ACC_WRITE_MASK);
> +	const u8 not_u = (u8)~BYTE_MASK(ACC_USER_MASK);
>  	const u8 u = BYTE_MASK(ACC_USER_MASK);
>  
>  	bool cr4_smep = kvm_read_cr4_bits(vcpu, X86_CR4_SMEP) != 0;
> @@ -4275,11 +4276,11 @@ static void update_permission_bitmask(struct kvm_vcpu *vcpu,
>  		 */
>  
>  		/* Faults from writes to non-writable pages */
> -		u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
> +		u8 wf = (pfec & PFERR_WRITE_MASK) ? not_w : 0;
>  		/* Faults from user mode accesses to supervisor pages */
> -		u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
> +		u8 uf = (pfec & PFERR_USER_MASK) ? not_u : 0;
>  		/* Faults from fetches of non-executable pages*/
> -		u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
> +		u8 ff = (pfec & PFERR_FETCH_MASK) ? not_x : 0;
>  		/* Faults from kernel mode fetches of user pages */
>  		u8 smepf = 0;
>  		/* Faults from kernel mode accesses of user pages */
>