Received: by 2002:a05:6902:102b:0:0:0:0 with SMTP id x11csp752709ybt; Fri, 10 Jul 2020 11:27:00 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxxauPm23ATsO5dZkv7SLCK/JSHYMmIhMggzGaYA7yO7iRpDGFC/NUW93WBLj1QSZuPTh89 X-Received: by 2002:a17:906:434c:: with SMTP id z12mr56368345ejm.33.1594405620235; Fri, 10 Jul 2020 11:27:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1594405620; cv=none; d=google.com; s=arc-20160816; b=JAaBwTR2+uWSWNi6M9wXYhaKchoJxpRLmL4VbI/2C4wWNe4ml+Rg1dcfvIqP71ZqXi dlLD9lqpsTP/4tJFfb1uu3RmkjWIiV4X7EijXO4BweS+MmMPdy3k+hBK7ABm43f8i4jq KqbNapXoaktq5kenKwWuCkZtKLuP61gmhScb7GR/X1Ywi9+V0vX5dW+XrF6Sj9lX71PG e1dE0Tiw4rhriR9cL9p9kTnITv4RQ6pWoVIjOGcK4ZqSBEUdtyVjZfXl4akN1/2MSA8F YpsigqwX0dpDTe7tXMvfGng0ZQrccf5Oe4d8nxo8yDeLaqItq7McbUsRIq85r31SYbm0 61pw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=mxvJ+Fty4K+1j3Y1eh6Bkx0XaBM+d2fy42IQC4UlRuo=; b=iI5YScWX0F1wXx1idaK1UoDojYonuUvBNPWmj5D3wT8kT2gjnQjvNjqwvmDsuP5+6y YVL98jWe3hUU+f9q1Kc3FY4PJ3ZGAgxEYQOy+M6zomVwF1c3iixUzpo3bPz3IZDm54Lh G+rCRaxLwSQQcwNVXaVT0nARJSl2NdCEIiqEkhGkooR5N82Ng3/P65ravMPUYhrRjKoX FDlAi8J/z5ug93+w8tHj9GzrBapVinJp/AxcIU0RuaBtHf95IZPeCO5Icr+pyOLfWNwi IEdL/a28NccDPVrQ/OZt8HMtsT+eemKGtWx+r7OVaWCUsICM6Zh1dE53vozbRzZ/Nr8j QklQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=NXWKXJ6G; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id d14si4802135edp.98.2020.07.10.11.26.35; Fri, 10 Jul 2020 11:27:00 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=NXWKXJ6G; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727782AbgGJSZ7 (ORCPT + 99 others); Fri, 10 Jul 2020 14:25:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52250 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726872AbgGJSZ7 (ORCPT ); Fri, 10 Jul 2020 14:25:59 -0400 Received: from mail-ed1-x543.google.com (mail-ed1-x543.google.com [IPv6:2a00:1450:4864:20::543]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1204DC08C5DC for ; Fri, 10 Jul 2020 11:25:59 -0700 (PDT) Received: by mail-ed1-x543.google.com with SMTP id z17so5367337edr.9 for ; Fri, 10 Jul 2020 11:25:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=mxvJ+Fty4K+1j3Y1eh6Bkx0XaBM+d2fy42IQC4UlRuo=; b=NXWKXJ6GQCKHQi13Z7gjc88eeS0hvvz4tGk2iYMcV5QSgNUJNYTnmS7BTx0yqrNtsz SFUJc1wRLVwL2p3HNbsfUm1WbLedW+TT+PyxxvwRydpPFVDDdOKPgKD0UySUkBqJrz5O bQrGlYWFWDaiujnsTzPboDZpoTdVs7RiQSGNM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mxvJ+Fty4K+1j3Y1eh6Bkx0XaBM+d2fy42IQC4UlRuo=; b=MZORuV9KYg0pzDIEYeOynpSk4yCnYof4O2IyPgKa4Yko9FbgX0rg7gLiz3Do+VCII6 ko83xJxj+EL8ZtIE3zbrFmkastigAlT/cI9gSVl26Oes4MHUYp12XdCBEiH2idAz9Y9z c15jJPiWteAiDpFQ9Gv4WfQ+FWxPheSDAW+Ok7QYiw51WQM7oGAw6RsBA8NglKwmsx0i Z2aZQA6cTKTb4WbhQyA5J5QeEMWNTjCWJDcAloHc3t7ufAj90jwbw/f+ZWeZP7klNfdI E7TXPAsyAMK+crCpxKoMovJQSG3KmdYrfwzB9GyRfXFwA1RtY9+SJ6XToToZ4NJFUEjB HD2Q== X-Gm-Message-State: AOAM531deAnfOOmBYM3I0lWth7Vqrw4Oggf/vvnPXo/+lpyqAyhslasp HoefC+S7g7iiZqXaTHQ510kq43tIhu8= X-Received: by 2002:a50:9f6a:: with SMTP id b97mr76752417edf.322.1594405557473; Fri, 10 Jul 2020 11:25:57 -0700 (PDT) Received: from mail-ej1-f41.google.com (mail-ej1-f41.google.com. [209.85.218.41]) by smtp.gmail.com with ESMTPSA id gu15sm4089614ejb.111.2020.07.10.11.25.56 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 10 Jul 2020 11:25:56 -0700 (PDT) Received: by mail-ej1-f41.google.com with SMTP id w16so7114882ejj.5 for ; Fri, 10 Jul 2020 11:25:56 -0700 (PDT) X-Received: by 2002:a17:906:c04d:: with SMTP id bm13mr53490468ejb.321.1594405555807; Fri, 10 Jul 2020 11:25:55 -0700 (PDT) MIME-Version: 1.0 References: <20200710002209.6757-1-apronin@chromium.org> <20200710114000.GD2614@linux.intel.com> In-Reply-To: <20200710114000.GD2614@linux.intel.com> From: Andrey Pronin Date: Fri, 10 Jul 2020 11:25:44 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] tpm: avoid accessing cleared ops during shutdown To: Jarkko Sakkinen Cc: peterhuewe@gmx.de, jgg@ziepe.ca, linux-integrity@vger.kernel.org, LKML , Guenter Roeck Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jul 10, 2020 at 4:40 AM Jarkko Sakkinen wrote: > > On Thu, Jul 09, 2020 at 05:22:09PM -0700, Andrey Pronin wrote: > > This patch prevents NULL dereferencing when using chip->ops while > > sending TPM2_Shutdown command if both tpm_class_shutdown handler and > > tpm_del_char_device are called during system shutdown. > > > > Both these handlers set chip->ops to NULL but don't check if it's > > already NULL when they are called before using it. > > > > This issue was revealed in Chrome OS after a recent set of changes > > to the unregister order for spi controllers, such as: > > b4c6230bb0ba spi: Fix controller unregister order > > f40913d2dca1 spi: pxa2xx: Fix controller unregister order > > and similar for other controllers. > > I'm not sure I fully understand the scenario. When does thi happen? This happens during system shutdown. Here a sample stack trace from the panic: BUG: unable to handle kernel NULL pointer dereference at 0000000000000058 ... Call Trace: tpm_transmit_cmd+0x21/0x7f tpm2_shutdown+0x84/0xc6 tpm_chip_unregister+0xa2/0xb9 cr50_spi_remove+0x19/0x26 spi_drv_remove+0x2a/0x42 device_release_driver_internal+0x123/0x1ec bus_remove_device+0xe8/0x111 device_del+0x1bf/0x319 ? spi_unregister_controller+0xfc/0xfc device_unregister+0x12/0x28 __unregister+0xe/0x12 device_for_each_child+0x79/0xb8 spi_unregister_controller+0x27/0xfc pxa2xx_spi_remove+0x45/0xb4 device_shutdown+0x181/0x1d3 kernel_restart+0x12/0x56 SyS_reboot+0x16a/0x1e7 do_syscall_64+0x6b/0xf7 entry_SYSCALL_64_after_hwframe+0x41/0xa6 > Why does not tpm_del_char_device need this? "Not" is a typo in the sentence above, right? tpm_del_char_device *does* need the fix. When tpm_class_shutdown is called it sets chip->ops to NULL. If tpm_del_char_device is called after that, it doesn't check if chip->ops is NULL (normal kernel API and char device API calls go through tpm_try_get_ops, but tpm_del_char_device doesn't) and proceeds to call tpm2_shutdown(), which tries sending the command and dereferences chip->ops. > The changes listed tell > me nothing. Why they have this effect? "spi: pxa2xx: Fix controller unregister order" adds a spi_unregister_controller call to pxa2xx_spi_remove, which internally calls tpm_del_char_device, which results in the stack trace leading to the panic above. > > I'm just trying to understand whether this could be a regression or > not. > > I neither understand what you mean by "and similar for other > controllers." There was a series of spi unregister order changes for various spi controllers, including the following: 1c6221b430a0 spi: bcm2835: Fix controller unregister order f40913d2dca1 spi: pxa2xx: Fix controller unregister order b4c6230bb0ba spi: Fix controller unregister order 54000d2e15e9 spi: dw: Fix controller unregister order c8f309db490e spi: bcm2835aux: Fix controller unregister order > > NAK for the reason that I don't understand what I'm merging. > > /Jarkko > > > > > Signed-off-by: Andrey Pronin > > --- > > drivers/char/tpm/tpm-chip.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > > index 8c77e88012e9..a410ca40a3c5 100644 > > --- a/drivers/char/tpm/tpm-chip.c > > +++ b/drivers/char/tpm/tpm-chip.c > > @@ -296,7 +296,7 @@ static int tpm_class_shutdown(struct device *dev) > > struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev); > > > > down_write(&chip->ops_sem); > > - if (chip->flags & TPM_CHIP_FLAG_TPM2) { > > + if (chip->ops && (chip->flags & TPM_CHIP_FLAG_TPM2)) { > > if (!tpm_chip_start(chip)) { > > tpm2_shutdown(chip, TPM2_SU_CLEAR); > > tpm_chip_stop(chip); > > @@ -479,7 +479,7 @@ static void tpm_del_char_device(struct tpm_chip *chip) > > > > /* Make the driver uncallable. */ > > down_write(&chip->ops_sem); > > - if (chip->flags & TPM_CHIP_FLAG_TPM2) { > > + if (chip->ops && (chip->flags & TPM_CHIP_FLAG_TPM2)) { > > if (!tpm_chip_start(chip)) { > > tpm2_shutdown(chip, TPM2_SU_CLEAR); > > tpm_chip_stop(chip); > > -- > > 2.25.1 > > -- Andrey