Re: [Xen-devel] [PATCH 05/11] tmem: don't access guest memory without using the accessors intended for this

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.09.12 at 18:50, Dan Magenheimer <dan.magenheimer@xxxxxxxxxx> wrote:
>>  From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Wednesday, September 05, 2012 6:37 AM
>> To: xen-devel
>> Cc: Dan Magenheimer; Zhenzhong Duan
>> Subject: [PATCH 05/11] tmem: don't access guest memory without using the 
> accessors intended for this
>> 
>> This is not permitted, not even for buffers coming from Dom0 (and it
>> would also break the moment Dom0 runs in HVM mode). An implication from
>> the changes here is that tmh_copy_page() can't be used anymore for
>> control operations calling tmh_copy_{from,to}_client() (as those pass
>> the buffer by virtual address rather than MFN).
>> 
>> Note that tmemc_save_get_next_page() previously didn't set the returned
>> handle's pool_id field, while the new code does. It need to be
>> confirmed that this is not a problem (otherwise the copy-out operation
>> will require further tmh_...() abstractions to be added).
>> 
>> Further note that the patch removes (rather than adjusts) an invalid
>> call to unmap_domain_page() (no matching map_domain_page()) from
>> tmh_compress_from_client() and adds a missing one to an error return
>> path in tmh_copy_from_client().
>> 
>> Finally note that the patch adds a previously missing return statement
>> to cli_get_page() (without which that function could de-reference a
>> NULL pointer, triggerable from guest mode).
>> 
>> This is part of XSA-15 / CVE-2012-3497.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> I'm a bit baffled by all the unrelated changes and cleanups, but
> they all appear to be valid fixes and, most importantly, the
> related security issues appear to be resolved.

Having gone through the patch once more, I'd be really
curious where you spotted unrelated changes (apart from
e.g. adding proper white space use on lines that needed
changing anyway).

With the size of the patch, and with this one having been
done when we still thought we would issue the patches
together with the advisory, I would really hope not to be
caught to have done unnecessary changes here (while
still preserving generic style of the code, see below).

> I'm also unable right now to plumb the depths of the guest copying
> macros so will have to trust Jan's good judgement.  One point in
> particular, it's difficult to determine if the following line is
> copying two bytes (wrong) or two uint64_t's (correct).
> 
>> +        tmh_copy_to_client_buf(buf, pool->uuid, 2);

With

#define tmh_copy_to_client_buf(clibuf, tmembuf, cnt) \
    copy_to_guest(guest_handle_cast(clibuf, void), tmembuf, cnt)

it is clear that it copies two elements of whatever tmembuf's
type is, in the case at hand uint64_t.

I would have preferred to use copy_to_guest() directly, but
that appeared to be against the spirit of the rest of the file,
which is why I added these new wrapper macros.

Jan


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