[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAFhGd8rohHPu+Zqr+w35=6=goUNC4ycvKk78N+7LSvNbnDnvuw@mail.gmail.com>
Date: Mon, 11 Sep 2023 15:01:16 -0700
From: Justin Stitt <justinstitt@...gle.com>
To: Dave Hansen <dave.hansen@...el.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>,
Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [PATCH] x86/tdx: refactor deprecated strncpy
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.
Powered by blists - more mailing lists