[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH V2] x86, amd_ucode: Support multiple container files appended together



On 06/24/2014 06:42 PM, Aravind Gopalakrishnan wrote:
On 6/24/2014 2:23 AM, Jan Beulich wrote:

@@ -272,14 +304,13 @@ static int get_ucode_from_buffer_amd(
    static int install_equiv_cpu_table(
      struct microcode_amd *mc_amd,
-    const uint32_t *buf,
-    size_t *offset)
+    const void *data,
+    size_t *curr_offset)
Is there any strong reason to rename "offset" to "curr_offset", ...

No. Have fixed this.

  {
+    uint32_t *buf = (uint32_t *) (data + *curr_offset);
      const struct mpbhdr *mpbuf = (const struct mpbhdr *)&buf[1];
  -    /* No more data */
-    if ( mpbuf->len + 12 >= *offset )
-        return -EINVAL;
Iirc you and Boris agreed that this check is pointless _here_. But I
doubt it can be removed without replacement elsewhere.

For single containers, this check made some sense earlier as we verify to see there is *some*
data beyond the equivalent_table structure.
Say, mpbuf->len=0 and we return error val; Due to the fact that we have already advanced *offset, cases when we reach EOF or *offset goes over bufsize is handled in container_fast_forward
function.

For multiple containers, we will always have at least two such container headers and hence,
mpbuf->len + 12 is always less than total_size

If first container for some reason is corrupted and exposes mpbuf->len=0, we return EINVAL
and forward to next container.
(This is infact one reason to  advance *offset earlier. See below)

Now, if the last container were to have mpbuf->len=0,
As Boris mentioned on earlier thread, we will
continue because 'if (0+12 >= tot_size) ' is false.
Here too, we will return EINVAL.

Again, advancing *offset early allows to workaround these issues.
And this check can be removed as a result.

Let's say we have a single container and the file got truncated (i.e. bufsize in cpu_request_microcode() is smaller than it should be). Aren't we now risking doing a memcpy out of too short a buffer?

-boris





_______________________________________________
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®.