linux-kernel - Re: [PATCH v3 2/4] x86/microcode: Fix __user annotations around generic_load

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]

Date:   Mon, 1 Apr 2019 19:53:46 +0200
From:   Jann Horn <jannh@...gle.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        Qiaowei Ren <qiaowei.ren@...el.com>,
        Mukesh Ojha <mojha@...eaurora.org>
Subject: Re: [PATCH v3 2/4] x86/microcode: Fix __user annotations around generic_load_microcode()

On Mon, Apr 1, 2019 at 7:30 PM Borislav Petkov <bp@...en8.de> wrote:
>
> On Fri, Mar 29, 2019 at 10:46:50PM +0100, Jann Horn wrote:
> > generic_load_microcode() deals with a pointer that can be either a kernel
> > pointer or a user pointer. Pass it around as a __user pointer so that it
> > can't be dereferenced accidentally while its address space is unknown.
> > Use explicit casts to convert between __user and __kernel to inform the
> > checker that these address space conversions are intentional.
> >
> > Signed-off-by: Jann Horn <jannh@...gle.com>
> > ---
> >  arch/x86/kernel/cpu/microcode/intel.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> > index 16936a24795c..e8ef65c275c7 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel.c
> > @@ -861,11 +861,13 @@ static enum ucode_state apply_microcode_intel(int cpu)
> >       return ret;
> >  }
> >
> > -static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
> > -                             int (*get_ucode_data)(void *, const void *, size_t))
> > +static enum ucode_state generic_load_microcode(int cpu,
> > +             const void __user *data, size_t size,
> > +             int (*get_ucode_data)(void *, const void __user *, size_t))
>
> Ok, how about something completely different?
>
> This ->get_ucode_data() BIOS-code-like contraption has always bugged me
> for being too ugly to live.
>
> How about we vmalloc() a properly sized buffer - both
> generic_load_microcode() callers have the size - and then hand that
> buffer into generic_load_microcode() ?
>
> That solves the __user annotation fun immediately and would simplify
> generic_load_microcode() additionally.
>
> The disadvantage would be having to vmalloc() a couple of... , I think
> it is megabytes, with that old loading method request_microcode_user()
> but then if vmalloc() fails, then it was clearly too big. I don't think
> the blob can ever be that big though, to fail vmalloc(), but I'm not
> going to bet on it...

Hm. request_microcode_fw() gets that buffer from
request_firmware_direct(), which does this:

        __module_get(THIS_MODULE);
        ret = _request_firmware(firmware_p, name, device, NULL, 0,
                                FW_OPT_UEVENT | FW_OPT_NO_WARN |
                                FW_OPT_NOFALLBACK);
        module_put(THIS_MODULE);
        return ret;

What is that module_get()/module_put() supposed to be good for? Are we
expecting that caller to do something ridiculous like calling
module_put() on us? This doesn't seem to make any sense.

And then _request_firmware() goes and ends up in places like
kernel_read_file(), which already use vmalloc().


Anyway, isn't this kind of thing exactly why we have that iov_iter
stuff? request_microcode_fw() can build an ITER_KVEC,
request_microcode_user() can build an ITER_IOVEC. And then
generic_load_microcode() can use something like copy_from_iter(). Does
that sound reasonable?