From: Oliver Upton <oupton@google.com>
To: kvmarm@lists.cs.columbia.edu
Cc: kvm@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
Peter Shier <pshier@google.com>,
Paolo Bonzini <pbonzini@redhat.com>,
stable@kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/4] KVM: arm64: vgic: Don't assume the VM debugfs directory exists
Date: Sat, 2 Apr 2022 22:39:34 +0000 [thread overview]
Message-ID: <YkjQpoS4Y+3+dwjp@google.com> (raw)
In-Reply-To: <20220402174044.2263418-2-oupton@google.com>
On Sat, Apr 02, 2022 at 05:40:41PM +0000, Oliver Upton wrote:
> Unfortunately, there is no guarantee that KVM was able to instantiate a
> debugfs directory for a particular VM. To that end, KVM shouldn't even
> attempt to create new debugfs files in this case. If the specified
> parent dentry is NULL, debugfs_create_file() will instantiate files at
> the root of debugfs.
>
> Since it is possible to create the vgic-state file outside of a VM
> directory, the file is not cleaned up when a VM is destroyed.
> Nonetheless, the corresponding struct kvm is freed when the VM is
> destroyed.
>
> Plug the use-after-free by plainly refusing to create vgic-state when
> KVM fails to create a VM debugfs dir.
>
> Cc: stable@kernel.org
> Fixes: 929f45e32499 ("kvm: no need to check return value of debugfs_create functions")
> Signed-off-by: Oliver Upton <oupton@google.com>
Thinking about this a bit more...
The game of whack-a-mole for other files that possibly have the same bug
could probably be avoided if kvm->debugfs_dentry is initialized to
PTR_ERR(-ENOENT) by default. That way there's no special error handling
that needs to be done in KVM as any attempt to create a new file will
bail.
Going to test and send out a v2 most likely, just want to make sure no
other use of debugfs in KVM will flip out from the change.
--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
WARNING: multiple messages have this Message-ID (diff)
From: Oliver Upton <oupton@google.com>
To: kvmarm@lists.cs.columbia.edu
Cc: kvm@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
James Morse <james.morse@arm.com>,
Alexandru Elisei <alexandru.elisei@arm.com>,
Suzuki K Poulose <suzuki.poulose@arm.com>,
linux-arm-kernel@lists.infradead.org,
Peter Shier <pshier@google.com>,
Ricardo Koller <ricarkol@google.com>,
Reiji Watanabe <reijiw@google.com>,
Paolo Bonzini <pbonzini@redhat.com>,
Sean Christopherson <seanjc@google.com>,
stable@kernel.org
Subject: Re: [PATCH 1/4] KVM: arm64: vgic: Don't assume the VM debugfs directory exists
Date: Sat, 2 Apr 2022 22:39:34 +0000 [thread overview]
Message-ID: <YkjQpoS4Y+3+dwjp@google.com> (raw)
In-Reply-To: <20220402174044.2263418-2-oupton@google.com>
On Sat, Apr 02, 2022 at 05:40:41PM +0000, Oliver Upton wrote:
> Unfortunately, there is no guarantee that KVM was able to instantiate a
> debugfs directory for a particular VM. To that end, KVM shouldn't even
> attempt to create new debugfs files in this case. If the specified
> parent dentry is NULL, debugfs_create_file() will instantiate files at
> the root of debugfs.
>
> Since it is possible to create the vgic-state file outside of a VM
> directory, the file is not cleaned up when a VM is destroyed.
> Nonetheless, the corresponding struct kvm is freed when the VM is
> destroyed.
>
> Plug the use-after-free by plainly refusing to create vgic-state when
> KVM fails to create a VM debugfs dir.
>
> Cc: stable@kernel.org
> Fixes: 929f45e32499 ("kvm: no need to check return value of debugfs_create functions")
> Signed-off-by: Oliver Upton <oupton@google.com>
Thinking about this a bit more...
The game of whack-a-mole for other files that possibly have the same bug
could probably be avoided if kvm->debugfs_dentry is initialized to
PTR_ERR(-ENOENT) by default. That way there's no special error handling
that needs to be done in KVM as any attempt to create a new file will
bail.
Going to test and send out a v2 most likely, just want to make sure no
other use of debugfs in KVM will flip out from the change.
--
Thanks,
Oliver
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: Oliver Upton <oupton@google.com>
To: kvmarm@lists.cs.columbia.edu
Cc: kvm@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
James Morse <james.morse@arm.com>,
Alexandru Elisei <alexandru.elisei@arm.com>,
Suzuki K Poulose <suzuki.poulose@arm.com>,
linux-arm-kernel@lists.infradead.org,
Peter Shier <pshier@google.com>,
Ricardo Koller <ricarkol@google.com>,
Reiji Watanabe <reijiw@google.com>,
Paolo Bonzini <pbonzini@redhat.com>,
Sean Christopherson <seanjc@google.com>,
stable@kernel.org
Subject: Re: [PATCH 1/4] KVM: arm64: vgic: Don't assume the VM debugfs directory exists
Date: Sat, 2 Apr 2022 22:39:34 +0000 [thread overview]
Message-ID: <YkjQpoS4Y+3+dwjp@google.com> (raw)
In-Reply-To: <20220402174044.2263418-2-oupton@google.com>
On Sat, Apr 02, 2022 at 05:40:41PM +0000, Oliver Upton wrote:
> Unfortunately, there is no guarantee that KVM was able to instantiate a
> debugfs directory for a particular VM. To that end, KVM shouldn't even
> attempt to create new debugfs files in this case. If the specified
> parent dentry is NULL, debugfs_create_file() will instantiate files at
> the root of debugfs.
>
> Since it is possible to create the vgic-state file outside of a VM
> directory, the file is not cleaned up when a VM is destroyed.
> Nonetheless, the corresponding struct kvm is freed when the VM is
> destroyed.
>
> Plug the use-after-free by plainly refusing to create vgic-state when
> KVM fails to create a VM debugfs dir.
>
> Cc: stable@kernel.org
> Fixes: 929f45e32499 ("kvm: no need to check return value of debugfs_create functions")
> Signed-off-by: Oliver Upton <oupton@google.com>
Thinking about this a bit more...
The game of whack-a-mole for other files that possibly have the same bug
could probably be avoided if kvm->debugfs_dentry is initialized to
PTR_ERR(-ENOENT) by default. That way there's no special error handling
that needs to be done in KVM as any attempt to create a new file will
bail.
Going to test and send out a v2 most likely, just want to make sure no
other use of debugfs in KVM will flip out from the change.
--
Thanks,
Oliver
next prev parent reply other threads:[~2022-04-02 22:39 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-02 17:40 [PATCH 0/4] KVM: arm64: Fix use-after-free in debugfs Oliver Upton
2022-04-02 17:40 ` Oliver Upton
2022-04-02 17:40 ` Oliver Upton
2022-04-02 17:40 ` [PATCH 1/4] KVM: arm64: vgic: Don't assume the VM debugfs directory exists Oliver Upton
2022-04-02 17:40 ` Oliver Upton
2022-04-02 17:40 ` Oliver Upton
2022-04-02 22:39 ` Oliver Upton [this message]
2022-04-02 22:39 ` Oliver Upton
2022-04-02 22:39 ` Oliver Upton
2022-04-02 17:40 ` [PATCH 2/4] KVM: Only log about debugfs directory collision once Oliver Upton
2022-04-02 17:40 ` Oliver Upton
2022-04-02 17:40 ` Oliver Upton
2022-04-04 17:33 ` Sean Christopherson
2022-04-04 17:33 ` Sean Christopherson
2022-04-04 17:33 ` Sean Christopherson
2022-04-04 17:57 ` Oliver Upton
2022-04-04 17:57 ` Oliver Upton
2022-04-04 17:57 ` Oliver Upton
2022-04-02 17:40 ` [PATCH 3/4] selftests: KVM: Don't leak GIC FD across dirty log test iterations Oliver Upton
2022-04-02 17:40 ` Oliver Upton
2022-04-02 17:40 ` Oliver Upton
2022-04-02 19:26 ` Jing Zhang
2022-04-02 19:26 ` Jing Zhang
2022-04-02 19:26 ` Jing Zhang
2022-04-02 17:40 ` [PATCH 4/4] selftests: KVM: Free the GIC FD when cleaning up in arch_timer Oliver Upton
2022-04-02 17:40 ` Oliver Upton
2022-04-02 17:40 ` Oliver Upton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YkjQpoS4Y+3+dwjp@google.com \
--to=oupton@google.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=maz@kernel.org \
--cc=pbonzini@redhat.com \
--cc=pshier@google.com \
--cc=stable@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.