| Message ID | 20220404182119.3561025-2-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Fix use-after-free in debugfs | expand |
Hi Oliver, On Mon, 04 Apr 2022 19:21:17 +0100, Oliver Upton <oupton@google.com> 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. > > For arm64, 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. > > Nip the problem in the bud for all possible errant debugfs file > creations by initializing kvm->debugfs_dentry to -ENOENT. In so doing, > debugfs_create_file() will fail instead of creating the file in the root > directory. > > 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> > --- > virt/kvm/kvm_main.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 70e05af5ebea..04a426e65cb8 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -932,7 +932,7 @@ static void kvm_destroy_vm_debugfs(struct kvm *kvm) > int kvm_debugfs_num_entries = kvm_vm_stats_header.num_desc + > kvm_vcpu_stats_header.num_desc; > > - if (!kvm->debugfs_dentry) > + if (!IS_ERR(kvm->debugfs_dentry)) > return; Shouldn't this condition be inverted? It certainly looks odd. > > debugfs_remove_recursive(kvm->debugfs_dentry); > @@ -955,6 +955,12 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd) > int kvm_debugfs_num_entries = kvm_vm_stats_header.num_desc + > kvm_vcpu_stats_header.num_desc; > > + /* > + * Force subsequent debugfs file creations to fail if the VM directory > + * is not created. > + */ > + kvm->debugfs_dentry = ERR_PTR(-ENOENT); > + > if (!debugfs_initialized()) > return 0; > > @@ -5479,7 +5485,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm) > } > add_uevent_var(env, "PID=%d", kvm->userspace_pid); > > - if (kvm->debugfs_dentry) { > + if (!IS_ERR(kvm->debugfs_dentry)) { > char *tmp, *p = kmalloc(PATH_MAX, GFP_KERNEL_ACCOUNT); > > if (p) { Thanks, M.
On Wed, Apr 06, 2022 at 08:10:00AM +0100, Marc Zyngier wrote: > Hi Oliver, > > On Mon, 04 Apr 2022 19:21:17 +0100, > Oliver Upton <oupton@google.com> 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. > > > > For arm64, 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. > > > > Nip the problem in the bud for all possible errant debugfs file > > creations by initializing kvm->debugfs_dentry to -ENOENT. In so doing, > > debugfs_create_file() will fail instead of creating the file in the root > > directory. > > > > 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> > > --- > > virt/kvm/kvm_main.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 70e05af5ebea..04a426e65cb8 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -932,7 +932,7 @@ static void kvm_destroy_vm_debugfs(struct kvm *kvm) > > int kvm_debugfs_num_entries = kvm_vm_stats_header.num_desc + > > kvm_vcpu_stats_header.num_desc; > > > > - if (!kvm->debugfs_dentry) > > + if (!IS_ERR(kvm->debugfs_dentry)) > > return; > > Shouldn't this condition be inverted? It certainly looks odd. Err... Yep, this is plain wrong. Let me fix this obvious mistake. -- Thanks, Oliver
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 70e05af5ebea..04a426e65cb8 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -932,7 +932,7 @@ static void kvm_destroy_vm_debugfs(struct kvm *kvm) int kvm_debugfs_num_entries = kvm_vm_stats_header.num_desc + kvm_vcpu_stats_header.num_desc; - if (!kvm->debugfs_dentry) + if (!IS_ERR(kvm->debugfs_dentry)) return; debugfs_remove_recursive(kvm->debugfs_dentry); @@ -955,6 +955,12 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd) int kvm_debugfs_num_entries = kvm_vm_stats_header.num_desc + kvm_vcpu_stats_header.num_desc; + /* + * Force subsequent debugfs file creations to fail if the VM directory + * is not created. + */ + kvm->debugfs_dentry = ERR_PTR(-ENOENT); + if (!debugfs_initialized()) return 0; @@ -5479,7 +5485,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm) } add_uevent_var(env, "PID=%d", kvm->userspace_pid); - if (kvm->debugfs_dentry) { + if (!IS_ERR(kvm->debugfs_dentry)) { char *tmp, *p = kmalloc(PATH_MAX, GFP_KERNEL_ACCOUNT); if (p) {
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. For arm64, 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. Nip the problem in the bud for all possible errant debugfs file creations by initializing kvm->debugfs_dentry to -ENOENT. In so doing, debugfs_create_file() will fail instead of creating the file in the root directory. 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> --- virt/kvm/kvm_main.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)