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, 4 Oct 2023 09:32:54 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Justin Stitt <justinstitt@...gle.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
	Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org, Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH v2] x86/tdx: replace deprecated strncpy with strtomem_pad


* Justin Stitt <justinstitt@...gle.com> wrote:

> strncpy works perfectly here in all cases, however, it is deprecated and
> as such we should prefer more robust and less ambiguous string apis.
> 
> Let's use `strtomem_pad` as this matches the functionality of `strncpy`
> and is _not_ deprecated.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@...r.kernel.org
> Cc: Kees Cook <keescook@...omium.org>
> Signed-off-by: Justin Stitt <justinstitt@...gle.com>
> ---
> Changes in v2:
> - update subject (thanks Kees)
> - update commit message (thanks Dave)
> - rebase onto mainline cbf3a2cb156a2c91
> - Link to v1: https://lore.kernel.org/r/20230911-strncpy-arch-x86-coco-tdx-tdx-c-v1-1-4b38155727f3@google.com
> ---
> Note: build-tested
> 
> Note: Ingo Molnar has some concerns about the comment being out of sync
> [1] but I believe the comment still has a place as we can still
> theoretically copy 64 bytes into our destination buffer without a
> NUL-byte. The extra information about the 65th byte being NUL may serve
> helpful to future travelers of this code. What do we think? I can drop
> the comment in a v3 if needed.

>  	/* VMM assumes '\0' in byte 65, if the message took all 64 bytes */
> -	strncpy(message.str, msg, 64);
> +	strtomem_pad(message.str, msg, '\0');

My concern was that with the old code it was obvious that the size
of message.str was 64 bytes - but I judged this based on the
patch context alone, which seemingly lost context due to the change.

In reality it's easy to see it when reading the code, because the
length definition is right before the code:

        union {
                /* Define register order according to the GHCI */
                struct { u64 r14, r15, rbx, rdi, rsi, r8, r9, rdx; };

                char str[64];
                ^^^^^^^^^^^^^
        } message;

        /* VMM assumes '\0' in byte 65, if the message took all 64 bytes */
        strtomem_pad(message.str, msg, '\0');


:-)

So no worries - I've applied your patch to tip:x86/mm as-is.

Thanks,

	Ingo

Powered by blists - more mailing lists