linux-kernel - Re: [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode

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:   Wed, 11 Mar 2020 14:01:07 +0100
From:   Pavel Machek <pavel@...x.de>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Ard Biesheuvel <ardb@...nel.org>,
        Ingo Molnar <mingo@...nel.org>, linux-efi@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering
 multiple pages in mixed mode

Hi!

> Currently, the mixed mode runtime service wrappers require that all by-ref
> arguments that live in the vmalloc space have a size that is a power of 2,
> and are aligned to that same value. While this is a sensible way to
> construct an object that is guaranteed not to cross a page boundary, it is
> overly strict when it comes to checking whether a given object violates
> this requirement, as we can simply take the physical address of the first
> and the last byte, and verify that they point into the same physical
> page.

Dunno. If start passing buffers that _sometime_ cross page boundaries,
we'll get hard to debug failures. Maybe original code is better
buecause it catches problems earlier?

Furthermore, all existing code should pass aligned, 2^n size buffers,
so we should not need this in stable?

> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -321,16 +321,13 @@ virt_to_phys_or_null_size(void *va, unsi
>  	if (virt_addr_valid(va))
>  		return virt_to_phys(va);
>  
> -	/*
> -	 * A fully aligned variable on the stack is guaranteed not to
> -	 * cross a page bounary. Try to catch strings on the stack by
> -	 * checking that 'size' is a power of two.
> -	 */
> -	bad_size = size > PAGE_SIZE || !is_power_of_2(size);
> +	pa = slow_virt_to_phys(va);
>  
> -	WARN_ON(!IS_ALIGNED((unsigned long)va, size) || bad_size);
> +	/* check if the object crosses a page boundary */
> +	if (WARN_ON((pa ^ (pa + size - 1)) & PAGE_MASK))
> +		return 0;

We don't really need to do this computation on pa, it would work on va
as well, right? It does not matter much, but old code worked that way.

Plus, strictly speaking, pa + size can overflow for huge sizes, and
test will return false negatives.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)