[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.

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.