linux-kernel - Re: [PATCH v4 02/24] powerpc/pseries: Fix alignment of PLPKS structures and buffers

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:   Thu, 26 Jan 2023 00:09:53 +1100
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Andrew Donnellan <ajd@...ux.ibm.com>,
        linuxppc-dev@...ts.ozlabs.org, linux-integrity@...r.kernel.org
Cc:     gregkh@...uxfoundation.org, gcwilson@...ux.ibm.com,
        linux-kernel@...r.kernel.org, nayna@...ux.ibm.com,
        ruscur@...sell.cc, zohar@...ux.ibm.com, gjoyce@...ux.ibm.com,
        sudhakar@...ux.ibm.com, bgray@...ux.ibm.com, erichte@...ux.ibm.com,
        joel@....id.au
Subject: Re: [PATCH v4 02/24] powerpc/pseries: Fix alignment of PLPKS
 structures and buffers

Andrew Donnellan <ajd@...ux.ibm.com> writes:
> A number of structures and buffers passed to PKS hcalls have alignment
> requirements, which could on occasion cause problems:
>
> - Authorisation structures must be 16-byte aligned and must not cross a
>   page boundary
>
> - Label structures must not cross page boundaries
>
> - Password output buffers must not cross page boundaries
>
> Round up the allocations of these structures/buffers to the next power of
> 2 to make sure this happens.

It's not the *next* power of 2, it's the *nearest* power of 2, including
the initial value if it's already a power of 2.

That in conjunction with slab's guarantee that power of 2 sized objects
are naturally aligned, and that the relevant structs are smaller than a
page, is what makes this actually work.

So I think the patch is fine, but the change log and comments probably
need to be a bit clearer.

cheers

> Reported-by: Benjamin Gray <bgray@...ux.ibm.com>
> Fixes: 2454a7af0f2a ("powerpc/pseries: define driver for Platform KeyStore")
> Signed-off-by: Andrew Donnellan <ajd@...ux.ibm.com>
> Reviewed-by: Russell Currey <ruscur@...sell.cc>
> Signed-off-by: Russell Currey <ruscur@...sell.cc>
>
> ---
>
> v3: Merge plpks fixes and signed update series with secvar series
>
> v4: Fix typo in commit message
>
>     Move up in series (npiggin)
> ---
>  arch/powerpc/platforms/pseries/plpks.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/plpks.c b/arch/powerpc/platforms/pseries/plpks.c
> index 9e85b6d85b0b..a01cf2ff140a 100644
> --- a/arch/powerpc/platforms/pseries/plpks.c
> +++ b/arch/powerpc/platforms/pseries/plpks.c
> @@ -126,7 +126,8 @@ static int plpks_gen_password(void)
>  	u8 *password, consumer = PKS_OS_OWNER;
>  	int rc;
>  
> -	password = kzalloc(maxpwsize, GFP_KERNEL);
> +	// The password must not cross a page boundary, so we align to the next power of 2
> +	password = kzalloc(roundup_pow_of_two(maxpwsize), GFP_KERNEL);
>  	if (!password)
>  		return -ENOMEM;
>  
> @@ -162,7 +163,9 @@ static struct plpks_auth *construct_auth(u8 consumer)
>  	if (consumer > PKS_OS_OWNER)
>  		return ERR_PTR(-EINVAL);
>  
> -	auth = kzalloc(struct_size(auth, password, maxpwsize), GFP_KERNEL);
> +	// The auth structure must not cross a page boundary and must be
> +	// 16 byte aligned. We align to the next largest power of 2
> +	auth = kzalloc(roundup_pow_of_two(struct_size(auth, password, maxpwsize)), GFP_KERNEL);
>  	if (!auth)
>  		return ERR_PTR(-ENOMEM);
>  
> @@ -196,7 +199,8 @@ static struct label *construct_label(char *component, u8 varos, u8 *name,
>  	if (component && slen > sizeof(label->attr.prefix))
>  		return ERR_PTR(-EINVAL);
>  
> -	label = kzalloc(sizeof(*label), GFP_KERNEL);
> +	// The label structure must not cross a page boundary, so we align to the next power of 2
> +	label = kzalloc(roundup_pow_of_two(sizeof(*label)), GFP_KERNEL);
>  	if (!label)
>  		return ERR_PTR(-ENOMEM);
>  
> -- 
> 2.39.0