|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2] microcode: Scan the initramfs payload for microcode blob.
>>> On 18.07.13 at 18:36, Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx> wrote:
> The option to turn this scan on/off is gated by the 'ucode'
> parameter. The options are now:
> 'initrd' Scan for the microcode in any multiboot payload.
> <index> Attempt to load microcode blob (not the cpio archive
> format) from the multiboot payload number.
>
> This option alters slightly the 'ucode' parameter by now allowing
> two parameters:
> ucode=[<index>,][initrd]
I don't really like this - it should permit a number _or_ "initrd", not
both (as that's meaningless).
Also I think the term "initrd" usually refers to the old style variant
not using cpio format (see init/initramfs.c:populate_rootfs() in the
Linux sources), hence "initramfs", "cpio", or "initrd-cpio" might be
better to avoid confusion.
> When this code works, the hypervisor ring buffer will have:
> (XEN) microcode: CPU0 updated from revision 0x25 to 0x28, date = 2012-04-24
> .. and so on for all the other CPUs.
> (This is an example from a SandyBridge CPU).
This is misleading - the code might work, and there still might not
be any such messages: Firmware may have loaded an up-to-date
ucode version already.
> void __init microcode_set_module(unsigned int idx)
> {
> ucode_mod_idx = idx;
> ucode_mod_forced = 1;
> }
> -
Please don't drop newlines between functions.
> +/*
> + * The format is '<integer>,[initrd]'. Both options are optional.
> + * If the EFI has forced which of the multiboot payloads is to be used,
> + * the <index> parsing will not be attempted.
> + */
> static void __init parse_ucode(char *s)
> {
> - if ( !ucode_mod_forced )
This conditional should remain here, now guarding the whole rest
of the function (perhaps by inverting it and bailing out early): If
there was a "ucode=" in the EFI config file, then that's what we
ought to use; no attempts should be made to override the admin's
choice.
> - ucode_mod_idx = simple_strtol(s, NULL, 0);
> + char *ss;
> +
> + do {
> + ss = strchr(s, ',');
> + if ( ss )
> + *ss = '\0';
> +
> + if ( !ucode_mod_forced && ucode_mod_idx == 0)
Coding style.
> +static __initdata const char * const ucodes[] =
> {"kernel/x86/microcode/GenuineIntel.bin",
Some gcc versions will cause errors mixing constant and non-constant
data placed in the same overridden section. Please drop the second
const.
> +
> "kernel/x86/microcode/AuthenticAMD.bin"};
I wonder anyway whether we wouldn't be better off using CPUID
function 0 output here to generate the name, and drop this pretty
contrived array approach (you don't use the array as such
anyway, i.e. you could as well inline the uses).
> +/*
> + * 8MB ought to be enough.
> + */
> +#define MAX_EARLY_CPIO_MICROCODE (8 << 20)
> +
> +void __init microcode_scan_module(
> + unsigned long *module_map,
> + const multiboot_info_t *mbi,
> + void *(*bootmap)(const module_t *))
> +{
> + module_t *mod = (module_t *)__va(mbi->mods_addr);
> + uint64_t *_blob_start;
> + unsigned long _blob_size;
> + struct cpio_data cd;
> + long offset;
> + const char *p = NULL;
> + int i;
> +
> + ucode_blob.size = 0;
> + if ( !ucode_scan )
> + return;
> +
> + if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
> + p = ucodes[1];
> + else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
> + p = ucodes[0];
> + else
> + return;
> +
> + /*
> + * Try all modules and see whichever could be the microcode blob.
> + */
> + for ( i = 0; i < mbi->mods_count; i++ )
Module 0 is unconditionally the Dom0 kernel image, so no need to
look at it.
> + {
> + if ( !test_bit(i, module_map) )
> + continue;
> +
> + _blob_start = bootmap(&mod[i]);
> + _blob_size = mod[i].mod_end;
> + if ( !_blob_start )
> + {
> + printk("Could not map multiboot module #%d (size: %ld)\n",
> + i, _blob_size);
> + return;
continue?
> + }
> + cd.data = NULL;
> + cd.size = 0;
> + cd = find_cpio_data(p, _blob_start, _blob_size, &offset /* don't
> care */);
Line length. (Is the comment really valuable?)
> + if ( cd.data )
> + {
> + /*
> + * This is an arbitrary check - it would be sad if the blob
> + * consumed most of the memory and did not allow guests
> + * to launch.
> + */
> + if ( cd.size > MAX_EARLY_CPIO_MICROCODE )
> + {
> + printk("Multiboot %d microcode payload too big! (%ld, we
> can do %d)\n",
> + i, cd.size, MAX_EARLY_CPIO_MICROCODE);
> + goto err;
> + }
> + ucode_blob.size = cd.size;
> + ucode_blob.data = xmalloc_bytes(cd.size);
> + if ( !ucode_blob.data )
> + goto err;
Again, no reason to bail entirely. Clear cd.data, put the memcpy()
that follows past an "else", and all is fine.
> static void __init _do_microcode_update(unsigned long data)
> {
> - microcode_update_cpu((void *)data, ucode_mod.mod_end);
> + void *_data = (void *)data;
> + size_t len = ucode_mod.mod_end;
> +
> + if ( ucode_blob.size )
> + {
> + _data = (void *)ucode_blob.data;
> + len = ucode_blob.size;
Here it becomes very visible why allowing both methods at the
same time is bad: What tells you that what you found in the
cpio is better than what was specified as an explicit module?
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -48,7 +48,7 @@ obj-y += radix-tree.o
> obj-y += rbtree.o
> obj-y += lzo.o
>
> -obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma
> unlzo,$(n).init.o)
> +obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma
> unlzo,$(n).init.o earlycpio.o)
I think you meant
obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo
earlycpio,$(n).init.o)
?
> +struct cpio_data __cpuinit find_cpio_data(const char *path, void *data,
struct cpio_data __init find_cpio_data(const char *path, const void *data,
> + size_t len, long *offset)
> +{
> + const size_t cpio_header_len = 8*C_NFIELDS - 2;
> + struct cpio_data cd = { NULL, 0, "" };
> + const char *p, *dptr, *nptr;
> + unsigned int ch[C_NFIELDS], *chp, v;
> + unsigned char c, x;
> + size_t mypathsize = strlen(path);
> + int i, j;
> +
> + p = data;
> +
> + while (len > cpio_header_len) {
> + if (!*p) {
> + /* All cpio headers need to be 4-byte aligned */
> + p += 4;
> + len -= 4;
> + continue;
> + }
> +
> + j = 6; /* The magic field is only 6 characters */
> + chp = ch;
> + for (i = C_NFIELDS; i; i--) {
> + v = 0;
> + while (j--) {
> + v <<= 4;
> + c = *p++;
> +
> + x = c - '0';
> + if (x < 10) {
> + v += x;
> + continue;
> + }
> +
> + x = (c | 0x20) - 'a';
> + if (x < 6) {
> + v += x + 10;
> + continue;
> + }
> +
> + goto quit; /* Invalid hexadecimal */
> + }
> + *chp++ = v;
> + j = 8; /* All other fields are 8 characters */
> + }
> +
> + if ((ch[C_MAGIC] - 0x070701) > 1)
> + goto quit; /* Invalid magic */
> +
> + len -= cpio_header_len;
> +
> + dptr = PTR_ALIGN(p + ch[C_NAMESIZE], 4);
> + nptr = PTR_ALIGN(dptr + ch[C_FILESIZE], 4);
> +
> + if (nptr > p + len || dptr < p || nptr < dptr)
> + goto quit; /* Buffer overrun */
> +
> + if ((ch[C_MODE] & 0170000) == 0100000 &&
> + ch[C_NAMESIZE] >= mypathsize &&
> + !memcmp(p, path, mypathsize)) {
> + *offset = (long)nptr - (long)data;
> + if (ch[C_NAMESIZE] - mypathsize >= MAX_CPIO_FILE_NAME) {
> + printk(
> + "File %s exceeding MAX_CPIO_FILE_NAME [%d]\n",
> + p, MAX_CPIO_FILE_NAME);
> + }
> + strlcpy(cd.name, p + mypathsize, MAX_CPIO_FILE_NAME);
> +
> + cd.data = (void *)dptr;
> + cd.size = ch[C_FILESIZE];
> + return cd; /* Found it! */
I realize that you took this from Linux, but it looks wrong
nevertheless: The code compares the name with the passed in
path up to the length of that path, and stores the remainder in
cd.name. Yet the caller doesn't ever look at cd.name, and
hence something like kernel/x86/microcode/GenuineIntel.bin.sig
would match too.
For the purposes here, you could make the check above
ch[C_NAMESIZE] == mypathsize and drop the name field from
struct cpio_data.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |