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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.09.12 at 17:44, Dan Magenheimer <dan.magenheimer@xxxxxxxxxx> wrote:
>> > 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).
> 
> 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.

With the below, I'm afraid you didn't review the patch
properly, or am still having problems understanding the
security aspects here.

> - changing NULL to tmh_cli_buf_null

The latter is not a simple #define of NULL.

> - changing parameter void *cva to tmem_cli_va_t clibuf,
>   which results in

Similarly, tmem_cli_va_t is not an equivalent of "void *".

>   - changing all lines that use that that parameter
>    which gave you the license to

Preserving the name of the parameter was just calling for
overlooking things. Hence, with the type change I also
changed the name (and where necessary introduced new
local variables with the old type and name, serving the
original purpose).

>    - cleanup the whitespace in all those lines

As said, I permitted myself to do this on lines I had to touch
anyway.

> - all code using -EFAULT that you changed to "< 0"
>   works correctly (though is admittedly fragile)

I don't think so - note that the changes were to "<= 0", as some
of the functions only return 1 as success indicator (and pass 0
apparently to indicate didn't do anything or some such - this
aspect of the original code was confusing me quite a bit, and
I can't exclude I broke something there).

> - inlining the use of the bool "tmemc"

Yes, this one could have been preserved, but (almost) all of its
uses would have required changes anyway.

> - the addition of scratch (which I think I understand may
>   patch a security hole undocumented in the commit comment?)

No, this was a requirement for fixing what the comment says is
being fixed: Not being allowed to directly access guest memory,
scratch space is needed for calling the compression functions.
The alternative would have been to pass guest handles to the
compression code, which would have implied touching that
code too. I think you agree that would have been a much worse
choice.

Jan


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