linux-kernel - 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]

Message-ID: <69643667-4564-0266-1b26-13833191842c@intel.com>
Date:   Mon, 11 Sep 2023 11:51:03 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Justin Stitt <justinstitt@...gle.com>,
        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>
Cc:     linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
        Kees Cook <keescook@...omium.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [PATCH] x86/tdx: refactor deprecated strncpy

On 9/11/23 11:27, Justin Stitt wrote:
> `strncpy` is deprecated and we should prefer more robust string apis.

I dunno.  It actually seems like a pretty good fit here.

> 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.";

I'm not really following this logic.

We need to do the following:

1. Make sure not to over write past the end of 'message'
2. Make sure not to over read past the end of 'msg'
3. Make sure not to leak stack data into the hypercall registers
   in the case of short strings.

strncpy() does #1, #2 and #3 just fine.

The resulting string does *NOT* need to be NULL-terminated.  See the
comment:

    /* VMM assumes '\0' in byte 65, if the message took all 64 bytes */

Are there cases where strncpy() doesn't NULL-terminate the string other
than when the buffer is full?

I actually didn't realize that strncpy() pads its output up to the full
size.  I wonder if Kirill used it intentionally or whether he got lucky
here. :)