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

[Dan Magenheimer <dan.magenheimer@xxxxxxxxxx>]

> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Subject: RE: [PATCH 05/11] tmem: don't access guest memory without using the 
> accessors intended for
> this
> 
> >>> 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).

Hi Jan --

Again, I am not criticizing the end result or any part of
the patch, just noting that some of it _could_ have been
separated to a different patch, which would have made it
_much_ more obvious what was the core fix for the security issue.
No need to reiterate your reasons, I'm only providing more
detail here because it sounds as if you are asking sincerely,
not defensively.

- changing NULL to tmh_cli_buf_null
- changing parameter void *cva to tmem_cli_va_t clibuf,
  which results in
  - changing all lines that use that that parameter
   which gave you the license to
   - cleanup the whitespace in all those lines
- all code using -EFAULT that you changed to "< 0"
  works correctly (though is admittedly fragile)
- inlining the use of the bool "tmemc"
- the addition of scratch (which I think I understand may
  patch a security hole undocumented in the commit comment?)

Again, all valid cleanups, and some may even decrease the
probability that future code changes will create more
security issues, but most are IMHO unnecessary to
fix the very specific security issue at hand.  Does that
make sense?

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

Again, just trying to be honest and helpful... it may be clear
to you, but I would venture to guess it is _not_ clear to the
vast majority of (even highly experienced) system programmers
who have not studied the guest copying code in gory detail.

IMHO, the guest copying code is incredible in what it does
and indecipherable below the first layer.  Only the page macros
in the Linux mm subsystem rival it in layers of obfuscation ;-)

Is there any detailed documentation about how it works?  If not,
it would be good to add some.

One last time: _not_ criticizing!

Dan


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