linux-kernel - Re: [GIT pull] core/urgent for v5.18-rc6

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: <CAHk-=wiEC-Jdx4iTP5NQ0a8stsU+SL0DjbTwhQ+mvPGNBXyMag@mail.gmail.com>
Date:   Sun, 8 May 2022 11:36:23 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Zhangfei Gao <zhangfei.gao@...mail.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Jean-Philippe Brucker <jean-philippe@...aro.org>,
        Jacob Pan <jacob.jun.pan@...ux.intel.com>,
        Dave Hansen <dave.hansen@...el.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "the arch/x86 maintainers" <x86@...nel.org>
Subject: Re: [GIT pull] core/urgent for v5.18-rc6

On Sun, May 8, 2022 at 11:09 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> Looks like it is
>
>  ->(*sva_bind)()
>     -> intel_svm_bind_mm()
>       -> mmu_notifier_register(&svm->notifier, mm)
>
> and yes, the mmu notifiers annoyingly end up doing an mmgrab [..]

Side note: quite independently of this mmgrab issue, I think the code
in question is *very* suspect and horrendously fragile.

In particular, the code ends up being called through things like this:

     handle = iommu_sva_bind_device(uacce->parent, current->mm, NULL);

and then that Intel svm.c code does this:

                svm->pasid = mm->pasid;
                svm->mm = mm;
                svm->flags = flags;

and saves off that mm pointer in the 'svm' structure.

AND IT NEVER TAKES ANY REFERENCE TO IT AT ALL!

It then does

        mm = svm->mm;

later at some unspecified time, and the 'mm' might long since have died.

In other words, the code works almost by accident - the only user of
the 'mm' pointer seems to be that mmu_notifier_register() thing, so it
basically treats the 'struct mm_struct' as something as a random
cookie.

And yes, the mmu notifiers do then take that mmgrab reference to the
mm, so it all works.

But it sure looks horrendously ugly. Saving off a 'struct mm_struct'
pointer with having basically an accidental reference to it is WRONG.

In fact, it will save off that pointer whether it then actually does
the mmu_notifier thing on it, because the code actually does

                [...]
                svm->mm = mm;
                svm->flags = flags;
                INIT_LIST_HEAD_RCU(&svm->devs);

                if (!(flags & SVM_FLAG_SUPERVISOR_MODE)) {
                        svm->notifier.ops = &intel_mmuops;
                        ret = mmu_notifier_register(&svm->notifier, mm);
                [...]

so that mmu_notifier_register() call is conditional. On the freeing
path, it then uses that "svm->notifier.ops" pointer as a "did we
register this thing or not" flag, so again - it all technically
*works*, but this is all horrendously ugly and wrong on so many
levels, keeping pointers around with very dubious reference counting
indeed.

               Linus