Messages in this thread | | | From | Jann Horn <> | Date | Mon, 1 Apr 2019 19:53:46 +0200 | 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@alien8.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@google.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?
| |