Re: [PATCH 02/22] libxc: introduce xc_dom_seg_to_ptr_pages

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

Andrew Cooper writes ("Re: [PATCH 02/22] libxc: introduce xc_dom_seg_to_ptr_pages"):
> On 10/06/13 14:40, Ian Jackson wrote:
> > Or to put it another way: doing it this way makes it easier to see
> > that the resulting code is correct.
> 
> I absolutely agree for unstable, but am arguing this around a minimal
> set of changes for a security fix.

The reasoning behind security fixes having a minimal set of changes
is as follows:

1. We want security fixes to have a low probability of mistakes
   (both regressions and failures to fix the whole problem).

2. Therefore we want security fixes to be easy to review.

3. Therefore, and directly from (1), security fixes should be as
   obviously correct as possible.

4. Normally the best way to make a patch or series more obviously
   correct is to make it shorter.

The goal of making security fixes short (4) exists only to serve the
goals of review (3) and correctness (1).  If it is easier to assure
correctness of a longer series, then that longer series is desirable.

As I say:
> > Or to put it another way: doing it this way makes it easier to see
> > that the resulting code is correct.

Indeed this whole series is much bigger, textually, than it could have
been.  Folding the patches into a single diff would make the result
"smaller" by a factor of two.  Using a different approach such as
trying to add specific range checking at every pointer computation
site might well have produced a smaller patch, but it would be much
harder to see whether the results were correct.

> In practice, I would suggest that xc_dom_seg_to_ptr() be updated to have
> the pages count, and all callsites updated appropriately.

When you say "have the pages count" what do you mean ?  You mean to
_take_ the pages count ?  But the pages count can usefully be computed
centrally in xc_dom_seg_to_ptr.

Ian.