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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.09.13 at 14:10, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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.

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@xxxxxxxxxx>
> CC: Keir Fraser <keir@xxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> ---
>  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




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel