linux-hardening - Re: [PATCH] x86/tdx: refactor deprecated strncpy

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: Fri, 29 Sep 2023 14:50:46 -0700
From: Kees Cook <keescook@...omium.org>
To: Dave Hansen <dave.hansen@...el.com>,
	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,
	Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [PATCH] x86/tdx: refactor deprecated strncpy

On Fri, Sep 29, 2023 at 02:27:39PM -0700, Dave Hansen wrote:
> On 9/29/23 11:33, Kees Cook wrote:
> > On Mon, 11 Sep 2023 18:27:25 +0000, Justin Stitt wrote:
> >> `strncpy` is deprecated and we should prefer more robust string apis.
> >>
> >> In this case, `message.str` is not expected to be NUL-terminated as it
> >> is simply a buffer of characters residing in a union which allows for
> >> named fields representing 8 bytes each. There is only one caller of
> >> `tdx_panic()` and they use a 59-length string for `msg`:
> >> |	const char *msg = "TD misconfiguration: SEPT_VE_DISABLE attribute must be set.";
> >>
> >> [...]
> > This appears to be trivially correct, so I can take it via my tree.
> 
> Sorry about that, I was being clear as mud as to what I wanted to see
> here.  I was hoping for another more clear changelog at least.
> 
> The changelog makes it sound like there's a problem with not
> NULL-terminating 'message.str' when there isn't.  That makes it hard to
> tell what the patch's goals are.

Ah! Thanks, sorry. I thought it was resolved.

> As far as I can tell, the code is 100% correct with either the existing
> strncpy() or strtomem_pad(), even with a >64-byte string.  This _is_
> unusual because the hypervisor is nice and doesn't require NULL termination.
> 
> Would there be anything wrong with a changelog like this?
> 
> 	strncpy() works perfectly here in all cases.  However, it _is_
> 	deprecated and unsafe in other cases and there is an effort to
> 	purge it from the code base to avoid problems elsewhere.
> 
> 	Replace strncpy() with an equivalent (in this case)
> 	strtomem_pad() which is not deprecated.
> 
> In other words, this fixes no bug.  But we should do it anyway.

Sounds good; thanks! Justin, can you respin with these changes?

-Kees

-- 
Kees Cook