[v2] x86/tdx: replace deprecated strncpy with strtomem_pad

Commit Message

Justin Stitt Oct. 3, 2023, 9:54 p.m. UTC

strncpy works perfectly here in all cases, however, it is deprecated and
as such we should prefer more robust and less ambiguous string apis.

Let's use `strtomem_pad` as this matches the functionality of `strncpy`
and is _not_ deprecated.

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90
Cc: [e-mail]
Cc: Kees Cook <[e-mail]>
Signed-off-by: Justin Stitt <[e-mail]>
---
Changes in v2:
- update subject (thanks Kees)
- update commit message (thanks Dave)
- rebase onto mainline cbf3a2cb156a2c91
- Link to v1: https://lore.kernel.org/r/[e-mail]
---
Note: build-tested

Note: Ingo Molnar has some concerns about the comment being out of sync
[1] but I believe the comment still has a place as we can still
theoretically copy 64 bytes into our destination buffer without a
NUL-byte. The extra information about the 65th byte being NUL may serve
helpful to future travelers of this code. What do we think? I can drop
the comment in a v3 if needed.

[1]: https://lore.kernel.org/all/[e-mail]/
---
 arch/x86/coco/tdx/tdx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


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

Best regards,
--
Justin Stitt <[e-mail]>

Comments

Kees Cook Oct. 3, 2023, 11:44 p.m. UTC | #1

On Tue, Oct 03, 2023 at 09:54:59PM +0000, Justin Stitt wrote:
> strncpy works perfectly here in all cases, however, it is deprecated and
> as such we should prefer more robust and less ambiguous string apis.
> 
> Let's use `strtomem_pad` as this matches the functionality of `strncpy`
> and is _not_ deprecated.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: [e-mail]
> Cc: Kees Cook <[e-mail]>
> Signed-off-by: Justin Stitt <[e-mail]>

Thanks for respinning this! And yeah, I'd agree the comment probably
should stay.

Reviewed-by: Kees Cook <[e-mail]>

Dave Hansen Oct. 3, 2023, 11:46 p.m. UTC | #2

On 10/3/23 14:54, Justin Stitt wrote:
> Note: Ingo Molnar has some concerns about the comment being out of sync
> [1] but I believe the comment still has a place as we can still
> theoretically copy 64 bytes into our destination buffer without a
> NUL-byte. The extra information about the 65th byte being NUL may serve
> helpful to future travelers of this code. What do we think? I can drop
> the comment in a v3 if needed.

The comment looks fine to me as you've left it.  It _might_ be better to
say something like:

	Empty space in 'message.str' needs to be overwritten
	but does not need to be NULL-terminated.

But I wouldn't bother messing with it any more.

Acked-by: Dave Hansen <[e-mail]>

I'll stick this into the tdx branch tomorrow unless someone has stronger
feelings about it.

Ingo Molnar Oct. 4, 2023, 7:32 a.m. UTC | #3

* Justin Stitt <[e-mail]> wrote:

> strncpy works perfectly here in all cases, however, it is deprecated and
> as such we should prefer more robust and less ambiguous string apis.
> 
> Let's use `strtomem_pad` as this matches the functionality of `strncpy`
> and is _not_ deprecated.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: [e-mail]
> Cc: Kees Cook <[e-mail]>
> Signed-off-by: Justin Stitt <[e-mail]>
> ---
> Changes in v2:
> - update subject (thanks Kees)
> - update commit message (thanks Dave)
> - rebase onto mainline cbf3a2cb156a2c91
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
> Note: build-tested
> 
> Note: Ingo Molnar has some concerns about the comment being out of sync
> [1] but I believe the comment still has a place as we can still
> theoretically copy 64 bytes into our destination buffer without a
> NUL-byte. The extra information about the 65th byte being NUL may serve
> helpful to future travelers of this code. What do we think? I can drop
> the comment in a v3 if needed.

>  	/* VMM assumes '\0' in byte 65, if the message took all 64 bytes */
> -	strncpy(message.str, msg, 64);
> +	strtomem_pad(message.str, msg, '\0');

My concern was that with the old code it was obvious that the size
of message.str was 64 bytes - but I judged this based on the
patch context alone, which seemingly lost context due to the change.

In reality it's easy to see it when reading the code, because the
length definition is right before the code:

        union {
                /* Define register order according to the GHCI */
                struct { u64 r14, r15, rbx, rdi, rsi, r8, r9, rdx; };

                char str[64];
                ^^^^^^^^^^^^^
        } message;

        /* VMM assumes '\0' in byte 65, if the message took all 64 bytes */
        strtomem_pad(message.str, msg, '\0');


:-)

So no worries - I've applied your patch to tip:x86/mm as-is.

Thanks,

	Ingo

Andy Shevchenko Feb. 7, 2024, 2:03 p.m. UTC | #4

On Wed, Oct 04, 2023 at 09:32:54AM +0200, Ingo Molnar wrote:

...

> > Note: Ingo Molnar has some concerns about the comment being out of sync
> > [1] but I believe the comment still has a place as we can still
> > theoretically copy 64 bytes into our destination buffer without a
> > NUL-byte. The extra information about the 65th byte being NUL may serve
> > helpful to future travelers of this code. What do we think? I can drop
> > the comment in a v3 if needed.
> 
> >  	/* VMM assumes '\0' in byte 65, if the message took all 64 bytes */
> > -	strncpy(message.str, msg, 64);
> > +	strtomem_pad(message.str, msg, '\0');
> 
> My concern was that with the old code it was obvious that the size
> of message.str was 64 bytes - but I judged this based on the
> patch context alone, which seemingly lost context due to the change.
> 
> In reality it's easy to see it when reading the code, because the
> length definition is right before the code:
> 
>         union {
>                 /* Define register order according to the GHCI */
>                 struct { u64 r14, r15, rbx, rdi, rsi, r8, r9, rdx; };
> 
>                 char str[64];
>                 ^^^^^^^^^^^^^
>         } message;
> 
>         /* VMM assumes '\0' in byte 65, if the message took all 64 bytes */
>         strtomem_pad(message.str, msg, '\0');

This comment and size of union seems not in agreement.
How does the previous code work if message indeed takes 64 bytes?
By luck?

Kees Cook Feb. 10, 2024, 7:19 a.m. UTC | #5

On Wed, Feb 07, 2024 at 04:03:35PM +0200, Andy Shevchenko wrote:
> On Wed, Oct 04, 2023 at 09:32:54AM +0200, Ingo Molnar wrote:
> 
> ...
> 
> > > Note: Ingo Molnar has some concerns about the comment being out of sync
> > > [1] but I believe the comment still has a place as we can still
> > > theoretically copy 64 bytes into our destination buffer without a
> > > NUL-byte. The extra information about the 65th byte being NUL may serve
> > > helpful to future travelers of this code. What do we think? I can drop
> > > the comment in a v3 if needed.
> > 
> > >  	/* VMM assumes '\0' in byte 65, if the message took all 64 bytes */
> > > -	strncpy(message.str, msg, 64);
> > > +	strtomem_pad(message.str, msg, '\0');
> > 
> > My concern was that with the old code it was obvious that the size
> > of message.str was 64 bytes - but I judged this based on the
> > patch context alone, which seemingly lost context due to the change.
> > 
> > In reality it's easy to see it when reading the code, because the
> > length definition is right before the code:
> > 
> >         union {
> >                 /* Define register order according to the GHCI */
> >                 struct { u64 r14, r15, rbx, rdi, rsi, r8, r9, rdx; };
> > 
> >                 char str[64];
> >                 ^^^^^^^^^^^^^
> >         } message;
> > 
> >         /* VMM assumes '\0' in byte 65, if the message took all 64 bytes */
> >         strtomem_pad(message.str, msg, '\0');
> 
> This comment and size of union seems not in agreement.

It does agree -- the comment could be more clear.

> How does the previous code work if message indeed takes 64 bytes?
> By luck?

It's saying "the non-existent 65th byte is assumed to be %NUL". As in,
this is treated as a C string, even if it uses all 64 bytes.

Patch

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;