Re: [Xen-devel] [PATCH 1 of 2] tools/libxc: Remus Checkpoint Compression

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Shriram Rajagopalan writes ("[Xen-devel] [PATCH 1 of 2] tools/libxc: Remus 
Checkpoint Compression"):
> [stuff]

Thanks for this.  I have some comments.

TBH I don't think "remus" is the right name for this functionality.
This checkpoint compression may be useful in non-Remus applications
(we won't know until we get some numbers) so it would be sensible to
name the functions and files accordingly.

> +        if (!buf->pages) {
> +            if (!(buf->pages = malloc(buf->compbuf_size))) {
> +                ERROR("Could not allocate compression buffer");
> +                return -1;
> +            }
> +        } else {
> +            if (!(ptmp = realloc(buf->pages, buf->compbuf_size))) {
> +                ERROR("Could not reallocate compression buffer");
> +                return -1;
> +            }
> +            buf->pages = ptmp;

realloc(0, some_nonzero_size) is legal and does what you would hope.
Using that would simplify this.  (Yes, I know the original code does
it the verbose way later too...)

> +static
> +int __uncompress(xc_interface *xch, char *destpage, unsigned long 
> *compbuf_pos,
> +                 char *compbuf, unsigned long compbuf_size)

Identifiers starting with "__" are reserved for the C implementation
and must not be used in Xen userspace libraries like libxc.  (The same
goes for identifiers starting with "_" with external linkage; although
those may be used for other purposes.)

> +typedef unsigned int data_t;

uint16_t surely.

> +    if (src[0].off == BEGIN)
> +    {
> +        *compbuf_pos += sizeof(struct marker);
> +        for (i = 1; (*compbuf_pos < compbuf_size) && (src[i].off >= 0);
> +             i++, *compbuf_pos += sizeof(struct marker))
> +            dest[src[i].off] = src[i].val;
> +    }
> +    else if (src[0].off == FULLPAGE)
> +    {
> +        *compbuf_pos += sizeof(struct marker) + XC_PAGE_SIZE;
> +        memcpy(destpage, (char *)&src[1], XC_PAGE_SIZE);
> +    }

I think there are some bugs here:

1. The accesses of src, and both arms of the conditional I quote
   above, seem to be able to read beyond the end of the compressed
   data buffer.

2. The value of src[i].off is used without checking that it's within
   the allowable range.

3. The code seems to be full of unaligned accesses.

When you have prepared a draft revised patch, can you please have
another Remus developer review the code for security problems ?
I would like to see them provide a formal reviewed-by/acked-by on your
resubmission.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel