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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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's not to say that I disagree that strict reading of the C standard
> may indeed yield this undefined, but I don't think we are to hunt
> down all such undefined-nesses. The tool should really stay away
> from hair-splitting, and concentrate on pointing out real issues.
>
> Jan
>
>> While Xen's implementation of memcpy will do the correct thing in this case,
>> any user trying to load a 0 length microcode blob deserves an -EINVAL.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>   xen/arch/x86/microcode_amd.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
>> index a3ceef8..2c767a9 100644
>> --- a/xen/arch/x86/microcode_amd.c
>> +++ b/xen/arch/x86/microcode_amd.c
>> @@ -202,7 +202,7 @@ static int get_ucode_from_buffer_amd(
>>           return -EINVAL;
>>       }
>>   
>> -    if ( (off + mpbuf->len) > bufsize )
>> +    if ( mpbuf->len == 0 || ((off + mpbuf->len) > bufsize) )
>>       {
>>           printk(KERN_ERR "microcode: Bad data in microcode data file\n");
>>           return -EINVAL;
>> -- 
>> 1.7.10.4
>
>