Re: [Xen-devel] [PATCH 02/22] libxc: introduce xc_dom_seg_to_ptr_pages

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/06/13 14:40, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH 02/22] libxc: introduce 
> xc_dom_seg_to_ptr_pages"):
>> With a view to security, why does this change need to be present?
>>
>> At the end of the v6 series, this new function has 1 caller from
>> xc_dom_load_elf_kernel() and 5 callers of the original function.
>>
>> >From looking at the semantics of the function, it either returns NULL,
>> or maps all requested pages (xc_dom_seg size rounded up to the nearest page)
>>
>> With that in mind, xc_dom_load_elf_kernel() can calculate elf->dest_size
>> given a successful mapping from xc_dom_seg_to_ptr(), without needing
>> this _pages() variant.
> This avoids duplicating the calculation of the number of pages
> allocated.  Duplication of logic is of course in general a bad idea.
>
> In this specific case there would be a risk that an error in one of
> the functions (or perhaps future changes) would result in a difference
> between the two calculations, which (if the error is in the
> unfortunate direction) would result in a security problem.
>
> In general I think it is best to always carry the pointer and length
> together so for an allocation/access function of this kind to return
> both the pointer and length.

I absolutely agree for unstable, but am arguing this around a minimal
set of changes for a security fix.

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

~Andrew

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


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