Re: [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>

Jan