Re: [Xen-devel] [PATCH v5 12/17] xen/libxc: sched: DOMCTL_*vcpuaffinity works with hard and soft affinity

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On mar, 2013-12-03 at 18:21 +0000, George Dunlap wrote:
> On 12/03/2013 10:06 AM, Jan Beulich wrote:
> >>>> On 03.12.13 at 11:02, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> >>>>> On 02.12.13 at 19:29, Dario Faggioli <dario.faggioli@xxxxxxxxxx> wrote:
> >>> +                goto setvcpuaffinity_out;
> >>> +
> >>> +            /*
> >>> +             * We both set a new affinity and report back to the caller 
> >>> what
> >>> +             * the scheduler will be effectively using.
> >>> +             */
> >>> +            if ( vcpuaff->flags & XEN_VCPUAFFINITY_HARD )
> >>> +            {
> >>> +                ret = xenctl_bitmap_to_bitmap(cpumask_bits(new_affinity),
> >>> +                                              &vcpuaff->cpumap_hard,
> >>> +                                              
> >>> vcpuaff->cpumap_hard.nr_bits);
> >>
> >> There's no code above range checking vcpuaff->cpumap_hard.nr_bits,
> >> yet xenctl_bitmap_to_bitmap() uses the passed in value to write into
> >> the array pointed to by the first argument. Why is this not
> >> xenctl_bitmap_to_cpumask() in the first place?
> >
> > And just to make it explicit - with fundamental flaws like this, I'm
> > not certain anymore whether we really ought to rush this series
> > in for 4.4.
> 
> I'm certainly getting nervous about the prospect.
>
Sory. :-/

> However, the above 
> bug would only be triggered by bad input from domain 0, right? 
>
Exactly, either at domain creation or `xl vcpu-pin' time.

> I suppose 
> even that would be a potential security issue in a highly disaggregated 
> environment.
> 
And, it's fixed here (again, just waiting to gather a bit more
feedback/ack before reposting):

 git://xenbits.xen.org/people/dariof/xen.git numa/per-vcpu-affinity-v6
 
http://xenbits.xen.org/gitweb/?p=people/dariof/xen.git;a=shortlog;h=refs/heads/numa/per-vcpu-affinity-v6

> Other bugs in this patch would be similar.  This path is taken on domain 
> creation IIUC; so bugs in this particular patch would probably either be 
> unexpected behavior of the affinities, or failure to handle unusual 
> input from a trusted source (domain 0).
> 
Indeed.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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