linux-kernel - Re: [RFC PATCH kernel] KVM: Stop leaking memory in debugfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YQlAl7/GSHWwkzEj@kroah.com>
Date:   Tue, 3 Aug 2021 15:11:51 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Alexey Kardashevskiy <aik@...abs.ru>,
        "Kernel Mailing List, Linux" <linux-kernel@...r.kernel.org>,
        kvm <kvm@...r.kernel.org>
Subject: Re: [RFC PATCH kernel] KVM: Stop leaking memory in debugfs

On Tue, Aug 03, 2021 at 02:52:17PM +0200, Paolo Bonzini wrote:
> On Tue, Aug 3, 2021 at 1:16 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> > On Fri, Jul 30, 2021 at 02:32:17PM +1000, Alexey Kardashevskiy wrote:
> > >       snprintf(dir_name, sizeof(dir_name), "%d-%d", task_pid_nr(current), fd);
> > >       kvm->debugfs_dentry = debugfs_create_dir(dir_name, kvm_debugfs_dir);
> > > +     if (IS_ERR_OR_NULL(kvm->debugfs_dentry)) {
> > > +             pr_err("Failed to create %s\n", dir_name);
> > > +             return 0;
> > > +     }
> >
> > It should not matter if you fail a debugfs call at all.
> >
> > If there is a larger race at work here, please fix that root cause, do
> > not paper over it by attempting to have debugfs catch the issue for you.
> 
> I don't think it's a race, it's really just a bug that is intrinsic in how
> the debugfs files are named.  You can just do something like this:
> 
> #include <unistd.h>
> #include <stdio.h>
> #include <fcntl.h>
> #include <sys/wait.h>
> #include <sys/ioctl.h>
> #include <linux/kvm.h>
> #include <stdlib.h>
> int main() {
>         int kvmfd = open("/dev/kvm", O_RDONLY);
>         int fd = ioctl(kvmfd, KVM_CREATE_VM, 0);
>         if (fork() == 0) {
>                 printf("before: %d\n", fd);
>                 sleep(2);
>         } else {
>                 close(fd);
>                 sleep(1);
>                 int fd = ioctl(kvmfd, KVM_CREATE_VM, 0);
>                 printf("after: %d\n", fd);
>                 wait(NULL);
>         }
> }
> 
> So Alexey's patch is okay and I've queued it, though with pr_warn_ratelimited
> instead of pr_err.

So userspace can create kvm resources with duplicate names?  That feels
wrong to me.

But if all that is "duplicate" is the debugfs kvm directory, why not ask
debugfs if it is already present before trying to create it again?  That
way you will not have debugfs complain about duplicate
files/directories.

thanks,

greg k-h