Message ID | 20231003-strncpy-arch-x86-coco-tdx-tdx-c-v2-1-0bd21174a217@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2a8e:b0:403:3b70:6f57 with SMTP id in14csp2373573vqb; Tue, 3 Oct 2023 14:56:08 -0700 (PDT) X-Google-Smtp-Source: AGHT+IESTS47QmwJbqKLMCNFP2Cl/E+IX/hGYOy7J9Z+TtzYSZqfqwiyeFjL3qSlOxC+2dMP6oQl X-Received: by 2002:a05:6358:5926:b0:14c:92ea:b500 with SMTP id g38-20020a056358592600b0014c92eab500mr800075rwf.4.1696370168216; Tue, 03 Oct 2023 14:56:08 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1696370168; cv=none; d=google.com; s=arc-20160816; b=FtN/4cYAAjplS68evVSskIMN9eGzIWyBWtIZR+lsqDtRFq2qne4lp5bWtqH96FJMcF I8TnKFw8Ukkfe4c1qXO3hTv1yXsL4Z3bgVr/of9vSuDNHLOAib23l8FFEIh6ETD0Mxx1 jGmKksviIa/G9SZq/Y+tFJPd8uw3VHbv720oK892Liuqr69bHnXh8Zl7cWVCS3Ooo0zf yLjckr6/Zk7vCLAne7lB5uFaPfSyCzxx6LfehqbNO/kgSgDzjwR9YTSFGKWAstPba9DA itreJWxmLA1eZad7ufwp6Lx0ddSEXW29OLW6cieAXACm05UsYCYy5b4tPCIuJ0rXjy3v H79Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:mime-version:date :dkim-signature; bh=a34O7D75QLNIsPbv9jkIuZDS2dnhXViv0FrAp24vkuM=; fh=V1xcIBPOhZwbQDw4xyGLULl7j/+VoUT6SDTXqn5x/y4=; b=Zm4VsJzTpyplET8cNpXtcVv/xc2Q3kI1nKmSvvYpkrWpnwpT5aOlCpurh//3WWEwXh ljFUywJN567fMRmkLKXI7v1SQo8JOs9+Xt1vNciWfTWucM7l8N4OQsFJQzecKiY+SNC6 VTg2UhMYGHeodvWCS0whVOmtV7YbJiXuLXQCHIxBr+uBojYXyxBZBqHgc1RirmuBLc/c gVc+jstddakzUmgvT9tMrcUGguYNbOMEyk4w4uPFe2+VEBco/7RFF4S2BHRs5HG9EgMb wvIoHR7kNR6JH+rAYv4UauKda4zMCpZ4NZjMa4RDv3RAqsgY1KDvKofOvxkmM9nK9ik1 oesg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=cwkjOnBp; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from agentk.vger.email (agentk.vger.email. [23.128.96.32]) by mx.google.com with ESMTPS id 83-20020a630056000000b00578db71453esi2360436pga.468.2023.10.03.14.56.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 03 Oct 2023 14:56:08 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=cwkjOnBp; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id E451580907AE; Tue, 3 Oct 2023 14:55:21 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232304AbjJCVzF (ORCPT <rfc822;chrisfriedt@gmail.com> + 17 others); Tue, 3 Oct 2023 17:55:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37886 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231822AbjJCVzE (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 3 Oct 2023 17:55:04 -0400 Received: from mail-yw1-x1149.google.com (mail-yw1-x1149.google.com [IPv6:2607:f8b0:4864:20::1149]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EB387A1 for <linux-kernel@vger.kernel.org>; Tue, 3 Oct 2023 14:55:00 -0700 (PDT) Received: by mail-yw1-x1149.google.com with SMTP id 00721157ae682-59bdb9fe821so22100777b3.0 for <linux-kernel@vger.kernel.org>; Tue, 03 Oct 2023 14:55:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1696370100; x=1696974900; darn=vger.kernel.org; h=cc:to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=a34O7D75QLNIsPbv9jkIuZDS2dnhXViv0FrAp24vkuM=; b=cwkjOnBp+W/U4LePjLOOq9YqvwuS6aMCmVWov5eBf8yXldJIvG5uFyG85nBHxV4Tq5 Cb4ola4eYWk5DzF3CLDCqDv10UF+5pBhaA0o5aysiLmqNwWUdBKxVc8eGMo1+a3bApIJ BJAIqV5XZ+xIshlU9NqlhTnK4bRTyhfQwAoCwGLenOT6KLI2j9MXVoTQTwsktr1J+0yf 3xQoqWeG5jijiKdjN3IL+/zKvmdcA5/eNcgnMLP0YWifGloRC0vAPnpVEhJilZTPqgcH jJJ4AVBWd8vFSkAX4IUmNeD06XMc+h8GbcLE/emdlvvn3eiGXbShj0WW7eDewKOBSH5e eDFg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696370100; x=1696974900; h=cc:to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=a34O7D75QLNIsPbv9jkIuZDS2dnhXViv0FrAp24vkuM=; b=IIAYnHMzsZ+854epH0KEVDnLqhj4p08NDIVgKmEXxOij5eGA+aiSeawv3eEyBP+zQQ qBjao2uqX5PKoNCJqxCZgmdkh0Hxq1rkiUBd1fZRKgE+eYys6hrmMXHd83Vrun/iuK7F ZmL3S6uDD8lXMfh6ZWSZsW8GYR4PIX8NFx1zMl3Z8e4G3ohapDfKh51GUKtSFfYD729F Sj9zJ3ypU2rSuDgwzWhvSg1K9oIAHPSzRuXH18mZWUDEwPb97spsHzjWQWSZ7gQ/v1F/ 3mEY0uzTw7gE22xFfSYvr2DijspBoS2ubHFCEbfbvggz7TPKF3WUgmuZpJ5FNHgNginj XEhA== X-Gm-Message-State: AOJu0YxCPNZz4h53TjGNFWcZVCNQ4c0bethabZn0GqKd9tcohoTS0CLD Fkuv02rFEnbS3Hwh6lhsoIcNBUFGB0WgMKTu1Q== X-Received: from jstitt-linux1.c.googlers.com ([fda3:e722:ac3:cc00:2b:ff92:c0a8:23b5]) (user=justinstitt job=sendgmr) by 2002:a5b:b50:0:b0:d80:904d:c211 with SMTP id b16-20020a5b0b50000000b00d80904dc211mr9088ybr.7.1696370100160; Tue, 03 Oct 2023 14:55:00 -0700 (PDT) Date: Tue, 03 Oct 2023 21:54:59 +0000 Mime-Version: 1.0 X-B4-Tracking: v=1; b=H4sIALKNHGUC/42NMQ6DMBAEv4KuzkXYBDBU+UdEgQ8DlhIO2ZYFQ vw9Di9IscVsMXOAN84aD212gDPRestLAnnLgOZ+mQzaITHIXBZ5IwT64BZad+wdzbipComJMQz bNcJG6Vw3VaW1GiBZVmdGu12FV5d4tj6w269gFL/3f3cUKPChCyXKspb1WDwn5ult7sQf6M7z/ AK7Or9wzwAAAA== X-Developer-Key: i=justinstitt@google.com; a=ed25519; pk=tC3hNkJQTpNX/gLKxTNQKDmiQl6QjBNCGKJINqAdJsE= X-Developer-Signature: v=1; a=ed25519-sha256; t=1696370099; l=1983; i=justinstitt@google.com; s=20230717; h=from:subject:message-id; bh=PfoJUZKcz8zggS6f4ZS1DBK4K8u1I0Mr9PkhUy/a6XA=; b=EOuTr5sNtf5yNIHhFwCHLi3CcldhMNdQHeDS7LFD2wpobtYnAgSMDEeeIbhtkyPOd5ho0eqOT /o+QShgmwW4AlnFbhjd9aWiTIgtw2Af/2mOWekgELZOOoQ0SyJ5p7DG X-Mailer: b4 0.12.3 Message-ID: <20231003-strncpy-arch-x86-coco-tdx-tdx-c-v2-1-0bd21174a217@google.com> Subject: [PATCH v2] x86/tdx: replace deprecated strncpy with strtomem_pad From: Justin Stitt <justinstitt@google.com> To: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com> Cc: linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org, Kees Cook <keescook@chromium.org>, Justin Stitt <justinstitt@google.com> Content-Type: text/plain; charset="utf-8" X-Spam-Status: No, score=-8.4 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Tue, 03 Oct 2023 14:55:22 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778773045326428633 X-GMAIL-MSGID: 1778773045326428633 |
Series |
[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: linux-hardening@vger.kernel.org
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
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%2FnB@gmail.com/
---
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 <justinstitt@google.com>
Comments
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: linux-hardening@vger.kernel.org > Cc: Kees Cook <keescook@chromium.org> > Signed-off-by: Justin Stitt <justinstitt@google.com> Thanks for respinning this! And yeah, I'd agree the comment probably should stay. Reviewed-by: Kees Cook <keescook@chromium.org>
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 <dave.hansen@linux.intel.com> I'll stick this into the tdx branch tomorrow unless someone has stronger feelings about it.
* Justin Stitt <justinstitt@google.com> 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: linux-hardening@vger.kernel.org > Cc: Kees Cook <keescook@chromium.org> > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > 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
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?
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.
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;