| Message ID | 20230911-strncpy-arch-x86-coco-tdx-tdx-c-v1-1-4b38155727f3@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | c9babd5d95abf3fae6e798605ce5cac98e08daf9 |
| Headers | show |
| Series | x86/tdx: refactor deprecated strncpy | expand |
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.
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>
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 <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
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.
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
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!
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;
`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(-) --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230911-strncpy-arch-x86-coco-tdx-tdx-c-98b0b966bb8d Best regards, -- Justin Stitt <justinstitt@google.com>