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
