From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 46BAAC4829A for ; Tue, 13 Feb 2024 16:42:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=3c0uSSDL+Cg8yYGM5IKKdjIdU5upJD+mjaIGZLmprJY=; b=3ZSQ+0M52g0D6u duHbiON6u2h22ho66DHf0aX3JUgKBa4Tv1bWJpcbtW1to9+zZWQAGxHww7AOwaY7dbdpgoTwBgx6B 2b2CLuPqA5X1YpfqsI64U/VGSNja7505mVk/mRRlu8A4CxbR7Pu+2EwABbuJP6TQ+i5Fs2BnydWH/ hT9w0nknkqTDpV5TKCK+8mEdBp68+I5ggsoUKAYOgltH6GwnIijoqxwYuFnxRVIk3l66CSJOsYAiH f1U4JVqWkMns+5Di815kgvRJIpEJY9hjCechBmXracZkevbvryYS3P1fTdQuihL3qoDz8uyGaF5+K /oUKwp8Dhe6QGScj/kzA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rZvre-0000000A0Ed-33d9; Tue, 13 Feb 2024 16:42:34 +0000 Received: from mail-wm1-x32f.google.com ([2a00:1450:4864:20::32f]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rZvrc-0000000A0Dw-1wli for linux-arm-kernel@lists.infradead.org; Tue, 13 Feb 2024 16:42:33 +0000 Received: by mail-wm1-x32f.google.com with SMTP id 5b1f17b1804b1-410acf9e776so62595e9.1 for ; Tue, 13 Feb 2024 08:42:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1707842549; x=1708447349; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=go1NDQqUFQni149mzcXDcSXrog8f9Zls+FdUA+jGdQk=; b=TiByT/yrP8dH3COTzf8qHwipDYSgYA4KRXTJ5qopCb+r5+n8eAu3lLmjKtBG3evofG RbR3YJt2dLk+RY8hx9DVpSfuZwsovzaiuWGFATQIzPjBCPqTMHszeqz8duZrMa1xFDm5 VrsTBfF8/UnjHuSfk4oPzyo4k5R4jKx/13k9EthHSKiSfeFZkvWLiDtWXCI+PjH2XyZg YdbFX2/oJSL9jcCWoG48KRdQGrrQMQ/EodGyO6L1r6HcMT1qcn5jLSQwwP+jcCJcOMhc UN0ilBf7oc1DsuTAbZvMf7iQLNrgCYHpoSrzH+OW10KD2vU1ji6tAmN0sVukO+uTg0Aa ePYw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707842549; x=1708447349; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=go1NDQqUFQni149mzcXDcSXrog8f9Zls+FdUA+jGdQk=; b=mjx2WkgF6M+uKdAMkTZyvE5jO9JLLxnfqmKnsSiG49RAuWOgizTEh1Wjx8eJlZHc7O m0J0XXIfxdslpWpnN5dWc0WbVRWqj3U9Guvc3a9S+BRbihBdGfAB6eMUrvd0RUnKRuE6 R9Dskqq8qC0KWKhO+7eWqxWelrVv9fnuP3FSLtpIj5XBzOXeJotM5VAnNZ/FhmNdY9YZ p+iP4QDcsNYlmaS+TCvSiwqa/tnuTddwEM2lBb5ykHxY5TlapghVd/a3mV03iQV2cj9s e0cKUloH2LYupqRc9xGul8DUwXbu0xEbfb0LRg3U/CNEI5b1vfjBs7O/clX0pqNH57pu 1YlA== X-Forwarded-Encrypted: i=1; AJvYcCUtqka9+D3Fb2lXLA5Rf+4+nOi7tajYYzA4UwhHtHbI0qRhnxXfra3q18kLL6RhQ01Adei3DZvq5oVp9D+LjXY62K3Btl5mnOb4Vqo8zr0UL7S7Uyk= X-Gm-Message-State: AOJu0Yxu/k8Ii5ec5qS3+InAWbIxPqhVQDPJjbkcBqgHXVA7Yl4DXKmw Z4xNCy4WZrYkZPW7wELEfIHfvgF5lHvYwzYGpQNvoaZTARXhs0277Ohl5UtXBQ== X-Google-Smtp-Source: AGHT+IEQ4OF+7VY3dkS4xJoz0W42M2A0tS6ekcpBsUb3AD8IA75L0XaTEOsjHGusJ1xpxWDlVsgqAg== X-Received: by 2002:a05:600c:3d8f:b0:411:b159:1705 with SMTP id bi15-20020a05600c3d8f00b00411b1591705mr16193wmb.4.1707842549229; Tue, 13 Feb 2024 08:42:29 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCXfwGzBn6/aurEiTGpszh2LCsNgtKpZaK4O7KSdW15zt3eyvpV9u23g+v3vDptqHLzRGeVsJ3SKDXfsnt7e+UtF+Sg4rkbKRmuHXInQ2d6aDLuPgPfHFtSk6yg3gWBnknLRoqabklqiNrX+hs+xrbDaqtwwCzlMABDgBvnXxEX+zOPPNyKByAKcJuPlMxGBjSoAZPCIm2WqZrckhftEokC1hml+Ax6bV4UHjDYOZxMzRnWSgpezBZClJ+ddCUB6Fr3ar4Q3R0SWOVgJKbeG6vFcXtgH7BNJxL3kklmxx2VWHradejD05VcsQ0+o594aFRBYQJ01EFU6WvfYCtoEsqlvUBpgyMhOVq07zak64IJC/UuWYPWYxhBQpgDXHV5ea/z8TFEMIxlpZU7WVztwZww0EZzj+9dgqdYpzBb08GhdwoAv3hp1gRxH60PsGN88JH5T8dsPkcCxNquHoz2n8DRFShFJgIk8FapAKRRBjYc5/sOZZtdr2eQ5I7TCvo2rarim+kXSXWKWPxpM0d37bSQy5q2KvHmw2yEYAg== Received: from google.com (161.126.77.34.bc.googleusercontent.com. [34.77.126.161]) by smtp.gmail.com with ESMTPSA id az10-20020adfe18a000000b0033b4f82b301sm10232741wrb.3.2024.02.13.08.42.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 13 Feb 2024 08:42:28 -0800 (PST) Date: Tue, 13 Feb 2024 16:42:27 +0000 From: Sebastian Ene To: Oliver Upton Cc: catalin.marinas@arm.com, gshan@redhat.com, james.morse@arm.com, mark.rutland@arm.com, maz@kernel.org, rananta@google.com, ricarkol@google.com, ryan.roberts@arm.com, shahuang@redhat.com, suzuki.poulose@arm.com, will@kernel.org, yuzenghui@huawei.com, kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel-team@android.com, vdonnefort@google.com Subject: Re: [PATCH v5 3/4] KVM: arm64: Register ptdump with debugfs on guest creation Message-ID: References: <20240207144832.1017815-2-sebastianene@google.com> <20240207144832.1017815-5-sebastianene@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240213_084232_536672_E098C26B X-CRM114-Status: GOOD ( 47.67 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Feb 13, 2024 at 12:56:20AM +0000, Oliver Upton wrote: > On Wed, Feb 07, 2024 at 02:48:32PM +0000, Sebastian Ene wrote: Hello Oliver, > > While arch/*/mem/ptdump handles the kernel pagetable dumping code, > > introduce KVM/ptdump which shows the guest stage-2 pagetables. The > > separation is necessary because most of the definitions from the > > stage-2 pagetable reside in the KVM path and we will be invoking > > functionality **specific** to KVM. > > > > When a guest is created, register a new file entry under the guest > > debugfs dir which allows userspace to show the contents of the guest > > stage-2 pagetables when accessed. > > > > Signed-off-by: Sebastian Ene > > --- > > arch/arm64/kvm/Kconfig | 13 ++++++ > > arch/arm64/kvm/Makefile | 1 + > > arch/arm64/kvm/debug.c | 7 ++++ > > arch/arm64/kvm/kvm_ptdump.h | 20 ++++++++++ > > arch/arm64/kvm/ptdump.c | 79 +++++++++++++++++++++++++++++++++++++ > > 5 files changed, 120 insertions(+) > > create mode 100644 arch/arm64/kvm/kvm_ptdump.h > > create mode 100644 arch/arm64/kvm/ptdump.c > > > > diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig > > index 6c3c8ca73e7f..28097dd72174 100644 > > --- a/arch/arm64/kvm/Kconfig > > +++ b/arch/arm64/kvm/Kconfig > > @@ -68,4 +68,17 @@ config PROTECTED_NVHE_STACKTRACE > > > > If unsure, or not using protected nVHE (pKVM), say N. > > > > +config PTDUMP_STAGE2_DEBUGFS > > + bool "Present the stage-2 pagetables to debugfs" > > + depends on PTDUMP_DEBUGFS && KVM > > + default n > > + help > > + Say Y here if you want to show the stage-2 kernel pagetables > > + layout in a debugfs file. This information is only useful for kernel developers > > + who are working in architecture specific areas of the kernel. > > + It is probably not a good idea to enable this feature in a production > > + kernel. > > + > > + If in doubt, say N. > > + > > endif # VIRTUALIZATION > > diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile > > index c0c050e53157..190eac17538c 100644 > > --- a/arch/arm64/kvm/Makefile > > +++ b/arch/arm64/kvm/Makefile > > @@ -23,6 +23,7 @@ kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \ > > vgic/vgic-its.o vgic/vgic-debug.o > > > > kvm-$(CONFIG_HW_PERF_EVENTS) += pmu-emul.o pmu.o > > +kvm-$(CONFIG_PTDUMP_STAGE2_DEBUGFS) += ptdump.o > > > > always-y := hyp_constants.h hyp-constants.s > > > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > > index 8725291cb00a..aef52836cd90 100644 > > --- a/arch/arm64/kvm/debug.c > > +++ b/arch/arm64/kvm/debug.c > > @@ -14,6 +14,8 @@ > > #include > > #include > > > > +#include > > + > > #include "trace.h" > > > > /* These are the bits of MDSCR_EL1 we may manipulate */ > > @@ -342,3 +344,8 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu) > > vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE); > > vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE); > > } > > + > > +int kvm_arch_create_vm_debugfs(struct kvm *kvm) > > +{ > > + return kvm_ptdump_guest_register(kvm); > > +} > > diff --git a/arch/arm64/kvm/kvm_ptdump.h b/arch/arm64/kvm/kvm_ptdump.h > > new file mode 100644 > > index 000000000000..a7c00a28481b > > --- /dev/null > > +++ b/arch/arm64/kvm/kvm_ptdump.h > > @@ -0,0 +1,20 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (C) Google, 2023 > > + * Author: Sebastian Ene > > + */ > > + > > +#ifndef __KVM_PTDUMP_H > > +#define __KVM_PTDUMP_H > > + > > +#include > > +#include > > + > > + > > +#ifdef CONFIG_PTDUMP_STAGE2_DEBUGFS > > +int kvm_ptdump_guest_register(struct kvm *kvm); > > +#else > > +static inline int kvm_ptdump_guest_register(struct kvm *kvm) { return 0; } > > +#endif /* CONFIG_PTDUMP_STAGE2_DEBUGFS */ > > + > > +#endif /* __KVM_PTDUMP_H */ > > diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c > > new file mode 100644 > > index 000000000000..a4e984da8aa7 > > --- /dev/null > > +++ b/arch/arm64/kvm/ptdump.c > > @@ -0,0 +1,79 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +// > > +// Debug helper used to dump the stage-2 pagetables of the system and their > > +// associated permissions. > > +// > > +// Copyright (C) Google, 2023 > > +// Author: Sebastian Ene > > Same comment as last time about ... the comment :) > > Should be of the form > > /* > * > */ > I forgot to update this, sorry. Let me fix it. > > +#include > > +#include > > +#include > > + > > +#include > > is this needed? > We only need so I will update this. > > +#include > > + > > + > > +static int kvm_ptdump_guest_open(struct inode *inode, struct file *file); > > +static int kvm_ptdump_guest_show(struct seq_file *m, void *); > > can you structure the file in a way to avoid forward declarations? > Sounds good, let me drop those. > > +static const struct file_operations kvm_ptdump_guest_fops = { > > + .open = kvm_ptdump_guest_open, > > + .read = seq_read, > > + .llseek = seq_lseek, > > + .release = single_release, > > +}; > > + > > +static int kvm_ptdump_guest_open(struct inode *inode, struct file *file) > > +{ > > + return single_open(file, kvm_ptdump_guest_show, inode->i_private); > > +} > > + > > Shouldn't we take a reference on the KVM struct at open to avoid UAF? > > struct kvm *kvm = inode->i_private; > > if (!kvm_get_kvm_safe(kvm)) > return -ENOENT; > > Then you can do a put on it at close(). > Thanks, I though that the kvm_destroy_vm_debugfs will keep spinning if there are opened paths to the debugfs entry, but I guess nothing prevents that from happening and the kvm struct can be removed behind our back. > > +static int kvm_ptdump_visitor(const struct kvm_pgtable_visit_ctx *ctx, > > + enum kvm_pgtable_walk_flags visit) > > +{ > > + struct pg_state *st = ctx->arg; > > + struct ptdump_state *pt_st = &st->ptdump; > > + > > + note_page(pt_st, ctx->addr, ctx->level, ctx->old); > > + return 0; > > +} > > + > > +static int kvm_ptdump_show_common(struct seq_file *m, > > + struct kvm_pgtable *pgtable, > > + struct pg_state *parser_state) > > +{ > > + struct kvm_pgtable_walker walker = (struct kvm_pgtable_walker) { > > + .cb = kvm_ptdump_visitor, > > + .arg = parser_state, > > + .flags = KVM_PGTABLE_WALK_LEAF, > > + }; > > + > > + return kvm_pgtable_walk(pgtable, 0, BIT(pgtable->ia_bits), &walker); > > +} > > + > > +static int kvm_ptdump_guest_show(struct seq_file *m, void *) > > +{ > > + struct kvm *guest_kvm = m->private; > > + struct kvm_s2_mmu *mmu = &guest_kvm->arch.mmu; > > + struct pg_state parser_state = {0}; > > + int ret; > > + > > + write_lock(&guest_kvm->mmu_lock); > > + ret = kvm_ptdump_show_common(m, mmu->pgt, &parser_state); > > + write_unlock(&guest_kvm->mmu_lock); > > + > > + return ret; > > +} > > + > > +int kvm_ptdump_guest_register(struct kvm *kvm) > > +{ > > + struct dentry *parent; > > + > > + parent = debugfs_create_file("stage2_page_tables", 0400, > > + kvm->debugfs_dentry, kvm, > > + &kvm_ptdump_guest_fops); > > + if (IS_ERR(parent)) > > + return PTR_ERR(parent); > > This makes the otherwise benign debugfs failure into something fatal for > VM creation, no? > > From the documentation on debugfs_create_file(): > > * NOTE: it's expected that most callers should _ignore_ the errors returned > * by this function. Other debugfs functions handle the fact that the "dentry" > * passed to them could be an error and they don't crash in that case. > * Drivers should generally work fine even if debugfs fails to init anyway. > > The fact that kvm_arch_create_vm_debugfs() has a return value is a bit > of an anti-pattern to begin with. > Ack, I will ignore the retun code then. > -- > Thanks, > Oliver Thanks, Sebastian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel