[PATCH] x86/tdx: refactor deprecated strncpy

[PATCH] x86/tdx: refactor deprecated strncpy

[Justin Stitt]

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

Given all this information, let's use `strtomem_pad` as this matches the
functionality of `strncpy` in this instances whilst being a more robust
and easier to understand interface.

Link: 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>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Note: build-tested
---
 arch/x86/coco/tdx/tdx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 1d6b863c42b0..2e1be592c220 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -119,7 +119,7 @@ static void __noreturn tdx_panic(const char *msg)
 	} message;
 
 	/* VMM assumes '\0' in byte 65, if the message took all 64 bytes */
-	strncpy(message.str, msg, 64);
+	strtomem_pad(message.str, msg, '\0');
 
 	args.r8  = message.r8;
 	args.r9  = message.r9;

---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230911-strncpy-arch-x86-coco-tdx-tdx-c-98b0b966bb8d

Best regards,
--
Justin Stitt <justinstitt@google.com>

Re: [PATCH] x86/tdx: refactor deprecated strncpy

[Dave Hansen]

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. :)

Re: [PATCH] x86/tdx: refactor deprecated strncpy

[Justin Stitt]

On Mon, Sep 11, 2023 at 11:51 AM Dave Hansen <dave.hansen@intel.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.

Re: [PATCH] x86/tdx: refactor deprecated strncpy

[Kees Cook]

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@intel.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@chromium.org>

-- 
Kees Cook

Re: [PATCH] x86/tdx: refactor deprecated strncpy

[Kees Cook]

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.

Applied to for-next/hardening, thanks!

[1/1] x86/tdx: refactor deprecated strncpy
      https://git.kernel.org/kees/c/e32c46753312

Take care,

-- 
Kees Cook

Re: [PATCH] x86/tdx: refactor deprecated strncpy

[Ingo Molnar]

* Kees Cook <keescook@chromium.org> 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.
> 
> Applied to for-next/hardening, thanks!
> 
> [1/1] x86/tdx: refactor deprecated strncpy
>       https://git.kernel.org/kees/c/e32c46753312

Please don't apply - Dave had some reservations, plus after the
change the comment would be now out of sync with the code ...

Also, we generally carry such patches in the x86 tree.

Thanks,

	Ingo

Re: [PATCH] x86/tdx: refactor deprecated strncpy

[Kees Cook]

On Fri, Sep 29, 2023 at 11:14:14PM +0200, Ingo Molnar wrote:
> 
> * Kees Cook <keescook@chromium.org> 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.
> > 
> > Applied to for-next/hardening, thanks!
> > 
> > [1/1] x86/tdx: refactor deprecated strncpy
> >       https://git.kernel.org/kees/c/e32c46753312
> 
> Please don't apply - Dave had some reservations, plus after the
> change the comment would be now out of sync with the code ...
> 
> Also, we generally carry such patches in the x86 tree.

I've dropped it now. Thanks!

-- 
Kees Cook

Re: [PATCH] x86/tdx: refactor deprecated strncpy

[Dave Hansen]

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.

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.

Re: [PATCH] x86/tdx: refactor deprecated strncpy

[Kees Cook]

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