| Message ID | 20200331100531.4294-10-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/ucode: Cleanup and fixes - Part 4/n (AMD) | expand |
On 31.03.2020 12:05, Andrew Cooper wrote: > @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz > * It's possible the data file has multiple matching ucode, > * lets keep searching till the latest version > */ > - while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size, > - &offset)) == 0 ) > + buf += offset; > + size -= offset; > { > - /* > - * If the new ucode covers current CPU, compare ucodes and store the > - * one with higher revision. > - */ > - if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) && > - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) ) > + while ( size ) > { > - xfree(saved); > - saved = mc_amd->mpb; > - } > - else > - { > - xfree(mc_amd->mpb); > - mc_amd->mpb = NULL; > - } > + const struct container_microcode *mc; > + > + if ( size < sizeof(*mc) || > + (mc = buf)->type != UCODE_UCODE_TYPE || > + size - sizeof(*mc) < mc->len || > + !verify_patch_size(mc->len) ) > + { > + printk(XENLOG_ERR "microcode: Bad microcode data\n"); > + error = -EINVAL; > + break; > + } > > - if ( offset >= size ) > - break; > + /* > + * If the new ucode covers current CPU, compare ucodes and store the > + * one with higher revision. > + */ > + if ( (microcode_fits(mc->patch) != MIS_UCODE) && > + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) ) > + { > + saved = mc->patch; > + saved_size = mc->len; > + } > > - /* > - * 1. Given a situation where multiple containers exist and correct > - * patch lives on a container that is not the last container. > - * 2. We match equivalent ids using find_equiv_cpu_id() from the > - * earlier while() (On this case, matches on earlier container > - * file and we break) > - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd, > - * buf, size, &offset)) == 0 ) > - * 4. Find correct patch using microcode_fits() and apply the patch > - * (Assume: apply_microcode() is successful) > - * 5. The while() loop from (3) continues to parse the binary as > - * there is a subsequent container file, but... > - * 6. ...a correct patch can only be on one container and not on any > - * subsequent ones. (Refer docs for more info) Therefore, we > - * don't have to parse a subsequent container. So, we can abort > - * the process here. > - * 7. This ensures that we retain a success value (= 0) to 'error' > - * before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to > - * false and returns -EINVAL. > - */ > - if ( offset + SECTION_HDR_SIZE <= size && > - *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) > - break; > + /* Move over the microcode blob. */ > + buf += sizeof(*mc) + mc->len; > + size -= sizeof(*mc) + mc->len; > + > + /* > + * Peek ahead. If we see the start of another container, we've > + * exhaused all microcode blobs in this container. Exit cleanly. > + */ > + if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC ) > + break; While, as already indicated, I agree with shrinking the big comment, I think point 6 is what wants retaining in some form - it's not obvious at all why a subsequent container couldn't contain a higher rev ucode than what we've found. That comment refers us to docs, but I couldn't find anything to this effect in PM Vol 2. Assuming this indeed documented and true, with the comment extended accordingly Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 31/03/2020 15:51, Jan Beulich wrote: > On 31.03.2020 12:05, Andrew Cooper wrote: >> @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz >> * It's possible the data file has multiple matching ucode, >> * lets keep searching till the latest version >> */ >> - while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size, >> - &offset)) == 0 ) >> + buf += offset; >> + size -= offset; >> { >> - /* >> - * If the new ucode covers current CPU, compare ucodes and store the >> - * one with higher revision. >> - */ >> - if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) && >> - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) ) >> + while ( size ) >> { >> - xfree(saved); >> - saved = mc_amd->mpb; >> - } >> - else >> - { >> - xfree(mc_amd->mpb); >> - mc_amd->mpb = NULL; >> - } >> + const struct container_microcode *mc; >> + >> + if ( size < sizeof(*mc) || >> + (mc = buf)->type != UCODE_UCODE_TYPE || >> + size - sizeof(*mc) < mc->len || >> + !verify_patch_size(mc->len) ) >> + { >> + printk(XENLOG_ERR "microcode: Bad microcode data\n"); >> + error = -EINVAL; >> + break; >> + } >> >> - if ( offset >= size ) >> - break; >> + /* >> + * If the new ucode covers current CPU, compare ucodes and store the >> + * one with higher revision. >> + */ >> + if ( (microcode_fits(mc->patch) != MIS_UCODE) && >> + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) ) >> + { >> + saved = mc->patch; >> + saved_size = mc->len; >> + } >> >> - /* >> - * 1. Given a situation where multiple containers exist and correct >> - * patch lives on a container that is not the last container. >> - * 2. We match equivalent ids using find_equiv_cpu_id() from the >> - * earlier while() (On this case, matches on earlier container >> - * file and we break) >> - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd, >> - * buf, size, &offset)) == 0 ) >> - * 4. Find correct patch using microcode_fits() and apply the patch >> - * (Assume: apply_microcode() is successful) >> - * 5. The while() loop from (3) continues to parse the binary as >> - * there is a subsequent container file, but... >> - * 6. ...a correct patch can only be on one container and not on any >> - * subsequent ones. (Refer docs for more info) Therefore, we >> - * don't have to parse a subsequent container. So, we can abort >> - * the process here. >> - * 7. This ensures that we retain a success value (= 0) to 'error' >> - * before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to >> - * false and returns -EINVAL. >> - */ >> - if ( offset + SECTION_HDR_SIZE <= size && >> - *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) >> - break; >> + /* Move over the microcode blob. */ >> + buf += sizeof(*mc) + mc->len; >> + size -= sizeof(*mc) + mc->len; >> + >> + /* >> + * Peek ahead. If we see the start of another container, we've >> + * exhaused all microcode blobs in this container. Exit cleanly. >> + */ >> + if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC ) >> + break; > While, as already indicated, I agree with shrinking the big comment, > I think point 6 is what wants retaining in some form - it's not > obvious at all why a subsequent container couldn't contain a higher > rev ucode than what we've found. That comment refers us to docs, but > I couldn't find anything to this effect in PM Vol 2. Assuming this > indeed documented and true, with the comment extended accordingly > Reviewed-by: Jan Beulich <jbeulich@suse.com> I think it is referring to the internal PPR, which isn't even the one we have access to. As to the multiple containers aspect, I've deliberately "fixed" that in patch 11 so we do scan all the way to the end. Its a much more obvious way to do things, even if the default case is to only provide a single container applicable to a specific family. ~Andrew
On 31.03.2020 16:55, Andrew Cooper wrote: > On 31/03/2020 15:51, Jan Beulich wrote: >> On 31.03.2020 12:05, Andrew Cooper wrote: >>> @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz >>> * It's possible the data file has multiple matching ucode, >>> * lets keep searching till the latest version >>> */ >>> - while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size, >>> - &offset)) == 0 ) >>> + buf += offset; >>> + size -= offset; >>> { >>> - /* >>> - * If the new ucode covers current CPU, compare ucodes and store the >>> - * one with higher revision. >>> - */ >>> - if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) && >>> - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) ) >>> + while ( size ) >>> { >>> - xfree(saved); >>> - saved = mc_amd->mpb; >>> - } >>> - else >>> - { >>> - xfree(mc_amd->mpb); >>> - mc_amd->mpb = NULL; >>> - } >>> + const struct container_microcode *mc; >>> + >>> + if ( size < sizeof(*mc) || >>> + (mc = buf)->type != UCODE_UCODE_TYPE || >>> + size - sizeof(*mc) < mc->len || >>> + !verify_patch_size(mc->len) ) >>> + { >>> + printk(XENLOG_ERR "microcode: Bad microcode data\n"); >>> + error = -EINVAL; >>> + break; >>> + } >>> >>> - if ( offset >= size ) >>> - break; >>> + /* >>> + * If the new ucode covers current CPU, compare ucodes and store the >>> + * one with higher revision. >>> + */ >>> + if ( (microcode_fits(mc->patch) != MIS_UCODE) && >>> + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) ) >>> + { >>> + saved = mc->patch; >>> + saved_size = mc->len; >>> + } >>> >>> - /* >>> - * 1. Given a situation where multiple containers exist and correct >>> - * patch lives on a container that is not the last container. >>> - * 2. We match equivalent ids using find_equiv_cpu_id() from the >>> - * earlier while() (On this case, matches on earlier container >>> - * file and we break) >>> - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd, >>> - * buf, size, &offset)) == 0 ) >>> - * 4. Find correct patch using microcode_fits() and apply the patch >>> - * (Assume: apply_microcode() is successful) >>> - * 5. The while() loop from (3) continues to parse the binary as >>> - * there is a subsequent container file, but... >>> - * 6. ...a correct patch can only be on one container and not on any >>> - * subsequent ones. (Refer docs for more info) Therefore, we >>> - * don't have to parse a subsequent container. So, we can abort >>> - * the process here. >>> - * 7. This ensures that we retain a success value (= 0) to 'error' >>> - * before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to >>> - * false and returns -EINVAL. >>> - */ >>> - if ( offset + SECTION_HDR_SIZE <= size && >>> - *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) >>> - break; >>> + /* Move over the microcode blob. */ >>> + buf += sizeof(*mc) + mc->len; >>> + size -= sizeof(*mc) + mc->len; >>> + >>> + /* >>> + * Peek ahead. If we see the start of another container, we've >>> + * exhaused all microcode blobs in this container. Exit cleanly. >>> + */ >>> + if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC ) >>> + break; >> While, as already indicated, I agree with shrinking the big comment, >> I think point 6 is what wants retaining in some form - it's not >> obvious at all why a subsequent container couldn't contain a higher >> rev ucode than what we've found. That comment refers us to docs, but >> I couldn't find anything to this effect in PM Vol 2. Assuming this >> indeed documented and true, with the comment extended accordingly >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > > I think it is referring to the internal PPR, which isn't even the one we > have access to. > > As to the multiple containers aspect, I've deliberately "fixed" that in > patch 11 so we do scan all the way to the end. Right, meanwhile I've seen this. But shouldn't patch 11 then adjust at least the "Exit cleanly" part of the comment? You're merely breaking the inner loop then ... Jan
On 31/03/2020 16:13, Jan Beulich wrote: > On 31.03.2020 16:55, Andrew Cooper wrote: >> On 31/03/2020 15:51, Jan Beulich wrote: >>> On 31.03.2020 12:05, Andrew Cooper wrote: >>>> @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz >>>> * It's possible the data file has multiple matching ucode, >>>> * lets keep searching till the latest version >>>> */ >>>> - while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size, >>>> - &offset)) == 0 ) >>>> + buf += offset; >>>> + size -= offset; >>>> { >>>> - /* >>>> - * If the new ucode covers current CPU, compare ucodes and store the >>>> - * one with higher revision. >>>> - */ >>>> - if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) && >>>> - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) ) >>>> + while ( size ) >>>> { >>>> - xfree(saved); >>>> - saved = mc_amd->mpb; >>>> - } >>>> - else >>>> - { >>>> - xfree(mc_amd->mpb); >>>> - mc_amd->mpb = NULL; >>>> - } >>>> + const struct container_microcode *mc; >>>> + >>>> + if ( size < sizeof(*mc) || >>>> + (mc = buf)->type != UCODE_UCODE_TYPE || >>>> + size - sizeof(*mc) < mc->len || >>>> + !verify_patch_size(mc->len) ) >>>> + { >>>> + printk(XENLOG_ERR "microcode: Bad microcode data\n"); >>>> + error = -EINVAL; >>>> + break; >>>> + } >>>> >>>> - if ( offset >= size ) >>>> - break; >>>> + /* >>>> + * If the new ucode covers current CPU, compare ucodes and store the >>>> + * one with higher revision. >>>> + */ >>>> + if ( (microcode_fits(mc->patch) != MIS_UCODE) && >>>> + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) ) >>>> + { >>>> + saved = mc->patch; >>>> + saved_size = mc->len; >>>> + } >>>> >>>> - /* >>>> - * 1. Given a situation where multiple containers exist and correct >>>> - * patch lives on a container that is not the last container. >>>> - * 2. We match equivalent ids using find_equiv_cpu_id() from the >>>> - * earlier while() (On this case, matches on earlier container >>>> - * file and we break) >>>> - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd, >>>> - * buf, size, &offset)) == 0 ) >>>> - * 4. Find correct patch using microcode_fits() and apply the patch >>>> - * (Assume: apply_microcode() is successful) >>>> - * 5. The while() loop from (3) continues to parse the binary as >>>> - * there is a subsequent container file, but... >>>> - * 6. ...a correct patch can only be on one container and not on any >>>> - * subsequent ones. (Refer docs for more info) Therefore, we >>>> - * don't have to parse a subsequent container. So, we can abort >>>> - * the process here. >>>> - * 7. This ensures that we retain a success value (= 0) to 'error' >>>> - * before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to >>>> - * false and returns -EINVAL. >>>> - */ >>>> - if ( offset + SECTION_HDR_SIZE <= size && >>>> - *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) >>>> - break; >>>> + /* Move over the microcode blob. */ >>>> + buf += sizeof(*mc) + mc->len; >>>> + size -= sizeof(*mc) + mc->len; >>>> + >>>> + /* >>>> + * Peek ahead. If we see the start of another container, we've >>>> + * exhaused all microcode blobs in this container. Exit cleanly. >>>> + */ >>>> + if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC ) >>>> + break; >>> While, as already indicated, I agree with shrinking the big comment, >>> I think point 6 is what wants retaining in some form - it's not >>> obvious at all why a subsequent container couldn't contain a higher >>> rev ucode than what we've found. That comment refers us to docs, but >>> I couldn't find anything to this effect in PM Vol 2. Assuming this >>> indeed documented and true, with the comment extended accordingly >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> I think it is referring to the internal PPR, which isn't even the one we >> have access to. >> >> As to the multiple containers aspect, I've deliberately "fixed" that in >> patch 11 so we do scan all the way to the end. > Right, meanwhile I've seen this. But shouldn't patch 11 then adjust at > least the "Exit cleanly" part of the comment? You're merely breaking > the inner loop then ... I'd still argue that "exit cleanly" is fine in context. Without it, the end of buffer case happens fine as size becomes 0 and terminates both loops, but in the case that there is a following container, without it we fail because of the "!= UCODE_UCODE_TYPE" check. ~Andrew
On 31.03.2020 17:47, Andrew Cooper wrote: > On 31/03/2020 16:13, Jan Beulich wrote: >> On 31.03.2020 16:55, Andrew Cooper wrote: >>> On 31/03/2020 15:51, Jan Beulich wrote: >>>> On 31.03.2020 12:05, Andrew Cooper wrote: >>>>> @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz >>>>> * It's possible the data file has multiple matching ucode, >>>>> * lets keep searching till the latest version >>>>> */ >>>>> - while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size, >>>>> - &offset)) == 0 ) >>>>> + buf += offset; >>>>> + size -= offset; >>>>> { >>>>> - /* >>>>> - * If the new ucode covers current CPU, compare ucodes and store the >>>>> - * one with higher revision. >>>>> - */ >>>>> - if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) && >>>>> - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) ) >>>>> + while ( size ) >>>>> { >>>>> - xfree(saved); >>>>> - saved = mc_amd->mpb; >>>>> - } >>>>> - else >>>>> - { >>>>> - xfree(mc_amd->mpb); >>>>> - mc_amd->mpb = NULL; >>>>> - } >>>>> + const struct container_microcode *mc; >>>>> + >>>>> + if ( size < sizeof(*mc) || >>>>> + (mc = buf)->type != UCODE_UCODE_TYPE || >>>>> + size - sizeof(*mc) < mc->len || >>>>> + !verify_patch_size(mc->len) ) >>>>> + { >>>>> + printk(XENLOG_ERR "microcode: Bad microcode data\n"); >>>>> + error = -EINVAL; >>>>> + break; >>>>> + } >>>>> >>>>> - if ( offset >= size ) >>>>> - break; >>>>> + /* >>>>> + * If the new ucode covers current CPU, compare ucodes and store the >>>>> + * one with higher revision. >>>>> + */ >>>>> + if ( (microcode_fits(mc->patch) != MIS_UCODE) && >>>>> + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) ) >>>>> + { >>>>> + saved = mc->patch; >>>>> + saved_size = mc->len; >>>>> + } >>>>> >>>>> - /* >>>>> - * 1. Given a situation where multiple containers exist and correct >>>>> - * patch lives on a container that is not the last container. >>>>> - * 2. We match equivalent ids using find_equiv_cpu_id() from the >>>>> - * earlier while() (On this case, matches on earlier container >>>>> - * file and we break) >>>>> - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd, >>>>> - * buf, size, &offset)) == 0 ) >>>>> - * 4. Find correct patch using microcode_fits() and apply the patch >>>>> - * (Assume: apply_microcode() is successful) >>>>> - * 5. The while() loop from (3) continues to parse the binary as >>>>> - * there is a subsequent container file, but... >>>>> - * 6. ...a correct patch can only be on one container and not on any >>>>> - * subsequent ones. (Refer docs for more info) Therefore, we >>>>> - * don't have to parse a subsequent container. So, we can abort >>>>> - * the process here. >>>>> - * 7. This ensures that we retain a success value (= 0) to 'error' >>>>> - * before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to >>>>> - * false and returns -EINVAL. >>>>> - */ >>>>> - if ( offset + SECTION_HDR_SIZE <= size && >>>>> - *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) >>>>> - break; >>>>> + /* Move over the microcode blob. */ >>>>> + buf += sizeof(*mc) + mc->len; >>>>> + size -= sizeof(*mc) + mc->len; >>>>> + >>>>> + /* >>>>> + * Peek ahead. If we see the start of another container, we've >>>>> + * exhaused all microcode blobs in this container. Exit cleanly. >>>>> + */ >>>>> + if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC ) >>>>> + break; >>>> While, as already indicated, I agree with shrinking the big comment, >>>> I think point 6 is what wants retaining in some form - it's not >>>> obvious at all why a subsequent container couldn't contain a higher >>>> rev ucode than what we've found. That comment refers us to docs, but >>>> I couldn't find anything to this effect in PM Vol 2. Assuming this >>>> indeed documented and true, with the comment extended accordingly >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> I think it is referring to the internal PPR, which isn't even the one we >>> have access to. >>> >>> As to the multiple containers aspect, I've deliberately "fixed" that in >>> patch 11 so we do scan all the way to the end. >> Right, meanwhile I've seen this. But shouldn't patch 11 then adjust at >> least the "Exit cleanly" part of the comment? You're merely breaking >> the inner loop then ... > > I'd still argue that "exit cleanly" is fine in context. Maybe; to me "exit" suggests more like being done with all processing / looping. I'm not going to insist - you're the native speaker. > Without it, the end of buffer case happens fine as size becomes 0 and > terminates both loops, but in the case that there is a following > container, without it we fail because of the "!= UCODE_UCODE_TYPE" check. Of course. Jan
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 980e61c547..ae1276988f 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -70,6 +70,11 @@ struct mpbhdr { uint32_t len; uint8_t data[]; }; +struct container_microcode { + uint32_t type; /* UCODE_UCODE_TYPE */ + uint32_t len; + struct microcode_header_amd patch[]; +}; /* * Microcode updates for different CPUs are distinguished by their @@ -280,52 +285,6 @@ static int apply_microcode(const struct microcode_patch *patch) return 0; } -static int get_ucode_from_buffer_amd( - struct microcode_amd *mc_amd, - const void *buf, - size_t bufsize, - size_t *offset) -{ - const struct mpbhdr *mpbuf = buf + *offset; - - /* No more data */ - if ( *offset >= bufsize ) - { - printk(KERN_ERR "microcode: Microcode buffer overrun\n"); - return -EINVAL; - } - - if ( mpbuf->type != UCODE_UCODE_TYPE ) - { - printk(KERN_ERR "microcode: Wrong microcode payload type field\n"); - return -EINVAL; - } - - if ( (*offset + mpbuf->len) > bufsize ) - { - printk(KERN_ERR "microcode: Bad data in microcode data file\n"); - return -EINVAL; - } - - if ( !verify_patch_size(mpbuf->len) ) - { - printk(XENLOG_ERR "microcode: patch size mismatch\n"); - return -EINVAL; - } - - mc_amd->mpb = xmemdup_bytes(mpbuf->data, mpbuf->len); - if ( !mc_amd->mpb ) - return -ENOMEM; - - pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n", - smp_processor_id(), bufsize, mpbuf->len, *offset, - mc_amd->mpb->processor_rev_id, mc_amd->mpb->patch_id); - - *offset += mpbuf->len + SECTION_HDR_SIZE; - - return 0; -} - static int scan_equiv_cpu_table( const void *data, size_t size_left, @@ -430,9 +389,9 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of static struct microcode_patch *cpu_request_microcode(const void *buf, size_t size) { struct microcode_amd *mc_amd; - struct microcode_header_amd *saved = NULL; + const struct microcode_header_amd *saved = NULL; struct microcode_patch *patch = NULL; - size_t offset = 0; + size_t offset = 0, saved_size = 0; int error = 0; unsigned int cpu = smp_processor_id(); const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz * It's possible the data file has multiple matching ucode, * lets keep searching till the latest version */ - while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size, - &offset)) == 0 ) + buf += offset; + size -= offset; { - /* - * If the new ucode covers current CPU, compare ucodes and store the - * one with higher revision. - */ - if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) && - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) ) + while ( size ) { - xfree(saved); - saved = mc_amd->mpb; - } - else - { - xfree(mc_amd->mpb); - mc_amd->mpb = NULL; - } + const struct container_microcode *mc; + + if ( size < sizeof(*mc) || + (mc = buf)->type != UCODE_UCODE_TYPE || + size - sizeof(*mc) < mc->len || + !verify_patch_size(mc->len) ) + { + printk(XENLOG_ERR "microcode: Bad microcode data\n"); + error = -EINVAL; + break; + } - if ( offset >= size ) - break; + /* + * If the new ucode covers current CPU, compare ucodes and store the + * one with higher revision. + */ + if ( (microcode_fits(mc->patch) != MIS_UCODE) && + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) ) + { + saved = mc->patch; + saved_size = mc->len; + } - /* - * 1. Given a situation where multiple containers exist and correct - * patch lives on a container that is not the last container. - * 2. We match equivalent ids using find_equiv_cpu_id() from the - * earlier while() (On this case, matches on earlier container - * file and we break) - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd, - * buf, size, &offset)) == 0 ) - * 4. Find correct patch using microcode_fits() and apply the patch - * (Assume: apply_microcode() is successful) - * 5. The while() loop from (3) continues to parse the binary as - * there is a subsequent container file, but... - * 6. ...a correct patch can only be on one container and not on any - * subsequent ones. (Refer docs for more info) Therefore, we - * don't have to parse a subsequent container. So, we can abort - * the process here. - * 7. This ensures that we retain a success value (= 0) to 'error' - * before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to - * false and returns -EINVAL. - */ - if ( offset + SECTION_HDR_SIZE <= size && - *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) - break; + /* Move over the microcode blob. */ + buf += sizeof(*mc) + mc->len; + size -= sizeof(*mc) + mc->len; + + /* + * Peek ahead. If we see the start of another container, we've + * exhaused all microcode blobs in this container. Exit cleanly. + */ + if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC ) + break; + } } if ( saved ) { - mc_amd->mpb = saved; - patch = mc_amd; + mc_amd->mpb = xmemdup_bytes(saved, saved_size); + if ( mc_amd->mpb ) + patch = mc_amd; + else + error = -ENOMEM; } else free_patch(mc_amd);
Just as on the Intel side, there is no point having get_ucode_from_buffer_amd() make $N memory allocations and free $N-1 of them. Delete get_ucode_from_buffer_amd() and rewrite the loop in cpu_request_microcode() to have 'saved' point into 'buf' until we finally decide to duplicate that blob and return it to our caller. Introduce a new struct container_microcode to simplify interpreting the container format. Doubly indent the logic to substantially reduce the churn in a later change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/cpu/microcode/amd.c | 138 +++++++++++++-------------------------- 1 file changed, 47 insertions(+), 91 deletions(-)