[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 4/4] x86/microcode_amd: Fail attempts to load a 0-length microcode blob.
On 09/24/2013 09:19 AM, Jan Beulich wrote: 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. 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. JanWhile 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |