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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 2 Apr 2019 12:01:34 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Jann Horn <jannh@...gle.com>
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 01, 2019 at 07:53:46PM +0200, Jann Horn wrote:
> 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.

Yah, the microcode thing used to be a module. Not anymore.

> 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?

/me doesn't know that interface, goes and looks...

You mean doing something like iov_iter_init() ... and then
copy_from_iter()? I'm looking at vhost_vsock_alloc_pkt() as an example
for a user of that interface.

The only thing I'm unsure about is the use case: that iov thing uses
a bunch of segments with separate lengths AFAICT which it copies
back'n'forth.

The loader does that sequentially in the sense that it parses the header
first, does some checks, then computes the size of the trailing patch
data which then copies again, see those ->get_ucode_data invocations.

Which would mean that the iov_* stuff would be always called with a
single segment of some length. Not that it is an issue, just saying.

That's why I suggested doing the trivial thing of copying the whole
buffer at once but I tend to prefer simple things so... :-)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.