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:   Thu, 14 Sep 2023 20:07:14 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Justin Stitt <justinstitt@...gle.com>
Cc:     Dave Hansen <dave.hansen@...el.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>, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org,
        Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [PATCH] x86/tdx: refactor deprecated strncpy

On Mon, Sep 11, 2023 at 03:01:16PM -0700, Justin Stitt wrote:
> On Mon, Sep 11, 2023 at 11:51 AM Dave Hansen <dave.hansen@...el.com> wrote:
> >
> > 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.
> 
> Right, to be clear, I do not suspect a bug in the current
> implementation. Rather, let's move towards a less ambiguous interface
> for maintainability's sake
> 
> >
> > 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. :)
> 
> Big reason to use strtomem_pad as it is more obvious about what it does.
> 
> I'd love more thoughts/testing here.

This looks like exactly the right conversion: strtomem_pad() will do 1,
2, and 3 (and does it unambiguously and without allowing for a
possible-wrong "size" parameter for the destination buffer).

Reviewed-by: Kees Cook <keescook@...omium.org>

-- 
Kees Cook