Re: [PATCH 4/4] x86/microcode_amd: Fail attempts to load a 0-length microcode blob.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 24/09/13 15:26, Boris Ostrovsky wrote:
> On 09/24/2013 09:19 AM, Jan Beulich wrote:
>>>>> On 24.09.13 at 14:10, Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> wrote:
>>> Coverity ID: 1055319
>>>
>>> Coverity identified that when passed a microcode header with a
>>> length field of
>>> 0, get_ucode_from_buffer_amd() would end up calling memcpy(NULL,
>>> data, 0)
>>> which is undefined behaviour.
>> I think that's at least questionable: memcpy(..., 0) can hardly be
>> anything but a no-op, no matter whether either of the two pointers
>> in fact is a NULL one.
>
> I think what this patch is trying to prevent is passing NULL pointer
> to memcpy,
> not length being zero (if you follow the logic in
> get_ucode_from_buffer_amd() you
> will see that destination buffer may not be allocated when length is
> zero).
>
> AMD APM says
>
>   REP. The REP prefix repeats its associated string instruction the
> number
>   of times specified in the counter register (rCX). It terminates the
>   repetition when the value in rCX reaches 0.
>
> So presumably rCX is checked before rDI and memcpy would indeed be a nop.
>
> Still, I think in this particular case (destination can be NULL) the
> patch is
> a good thing since we usually (often) check before passing NULL
> pointer to
> routines. Alternatively, we can check 'if (mc_amd->mpb != NULL)'.
>
> -boris

That may not work as expected.

The subtalty here is that we only allocate memory for mc_amd->mpb if
mc_amd->mpb_size is less than mpbuf->len.

mc_amd->mpb is unconditionally NULL before the first allocation, and
mc_amd->mpb_size is 0 (from the caller)

Therefore, a user passing 0 for mpbuf->len causes the routine to choose
not to allocate any memory, then copy 0 bytes into it.

~Andrew