Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Sender: Ingo Molnar <mingo.kernel.org@gmail.com>
Date:   Wed, 4 Oct 2023 09:32:54 +0200
From:   Ingo Molnar <mingo@kernel.org>
To:     Justin Stitt <justinstitt@google.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org, Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH v2] x86/tdx: replace deprecated strncpy with strtomem_pad
Message-ID: <ZR0VJsgA6g0Wk4dq@gmail.com>
References: <20231003-strncpy-arch-x86-coco-tdx-tdx-c-v2-1-0bd21174a217@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231003-strncpy-arch-x86-coco-tdx-tdx-c-v2-1-0bd21174a217@google.com>
Precedence: bulk


* Justin Stitt <justinstitt@google.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@vger.kernel.org
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Justin Stitt <justinstitt@google.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