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: [email protected]
Cc: Kees Cook <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
Changes in v2:
- update subject (thanks Kees)
- update commit message (thanks Dave)
- rebase onto mainline cbf3a2cb156a2c91
- Link to v1: https://lore.kernel.org/r/20230911-strncpy-arch-x86-coco-tdx-tdx-c-v1-1-4b38155727f3@google.com
---
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/ZRc+JqO7XvyHg%[email protected]/
---
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: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
change-id: 20230911-strncpy-arch-x86-coco-tdx-tdx-c-98b0b966bb8d
Best regards,
--
Justin Stitt <[email protected]>
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: [email protected]
> Cc: Kees Cook <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
Thanks for respinning this! And yeah, I'd agree the comment probably
should stay.
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
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 <[email protected]>
I'll stick this into the tdx branch tomorrow unless someone has stronger
feelings about it.
* Justin Stitt <[email protected]> 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: [email protected]
> Cc: Kees Cook <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> Changes in v2:
> - update subject (thanks Kees)
> - update commit message (thanks Dave)
> - rebase onto mainline cbf3a2cb156a2c91
> - Link to v1: https://lore.kernel.org/r/20230911-strncpy-arch-x86-coco-tdx-tdx-c-v1-1-4b38155727f3@google.com
> ---
> 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
The following commit has been merged into the x86/mm branch of tip:
Commit-ID: c9babd5d95abf3fae6e798605ce5cac98e08daf9
Gitweb: https://git.kernel.org/tip/c9babd5d95abf3fae6e798605ce5cac98e08daf9
Author: Justin Stitt <[email protected]>
AuthorDate: Tue, 03 Oct 2023 21:54:59
Committer: Ingo Molnar <[email protected]>
CommitterDate: Wed, 04 Oct 2023 09:34:07 +02:00
x86/tdx: Replace deprecated strncpy() with strtomem_pad()
strncpy() works perfectly here in all cases, however, it is deprecated and
as such we should prefer more robust and less ambiguous string APIs:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
Let's use strtomem_pad() as this matches the functionality of strncpy()
and is _not_ deprecated.
Signed-off-by: Justin Stitt <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Acked-by: Dave Hansen <[email protected]>
Link: https://github.com/KSPP/linux/issues/90
Link: https://lore.kernel.org/r/20231003-strncpy-arch-x86-coco-tdx-tdx-c-v2-1-0bd21174a217@google.com
---
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 1d6b863..2e1be59 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;
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?
--
With Best Regards,
Andy Shevchenko
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.
--
Kees Cook