| Message ID | 570B0121.60306@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 11, 2016 at 09:42:57AM +0800, Zhenzhong Duan wrote: > It's tool's duty to pass a correct cpumap to XEN. On a host with less than > 64 > CPUS, it just shows below error. > > [root@localhost /]# xm vcpu-pin 3 all all > Error: Cannot pin vcpu: 0 to cpu: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, > 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, > 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, > 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63] - (22, 'Invalid > argument') > This error clearly shows that the upper layer is not passing down the right cpumap. So this example along won't convince me to that this is a bug in python binding. It's free to reject input that it deems wrong. I guess the question is more or less what should it do in such situation. So ... > The fix make it same as in xl code. > Is there reference to this? I guess the commit message or mail thread will shed more light on the expected behaviour. BTW please CC relevant maintainers in the future, thanks. Wei. > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com> > --- > tools/python/xen/lowlevel/xc/xc.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/tools/python/xen/lowlevel/xc/xc.c > b/tools/python/xen/lowlevel/xc/xc.c > index c40a4e9..2f4898d 100644 > --- a/tools/python/xen/lowlevel/xc/xc.c > +++ b/tools/python/xen/lowlevel/xc/xc.c > @@ -243,13 +243,15 @@ static PyObject *pyxc_vcpu_setaffinity(XcObject *self, > for ( i = 0; i < PyList_Size(cpulist); i++ ) > { > long cpu = PyInt_AsLong(PyList_GetItem(cpulist, i)); > - if ( cpu < 0 || cpu >= nr_cpus ) > + if ( cpu < 0 ) > { > free(cpumap); > errno = EINVAL; > PyErr_SetFromErrno(xc_error_obj); > return NULL; > } > + if ( cpu >= nr_cpus ) > + continue; > cpumap[cpu / 8] |= 1 << (cpu % 8); > } > } > -- > 1.7.3 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
? 2016/4/11 19:27, Wei Liu ??: > On Mon, Apr 11, 2016 at 09:42:57AM +0800, Zhenzhong Duan wrote: >> It's tool's duty to pass a correct cpumap to XEN. On a host with less than >> 64 >> CPUS, it just shows below error. >> >> [root@localhost /]# xm vcpu-pin 3 all all >> Error: Cannot pin vcpu: 0 to cpu: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, >> 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, >> 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, >> 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63] - (22, 'Invalid >> argument') >> > This error clearly shows that the upper layer is not passing down the > right cpumap. So this example along won't convince me to that this is a > bug in python binding. It's free to reject input that it deems wrong. > > I guess the question is more or less what should it do in such > situation. So ... > >> The fix make it same as in xl code. >> > Is there reference to this? I guess the commit message or mail thread > will shed more light on the expected behaviour. Calling path: main_vcpupin->cpurange_parse->update_cpumap_range->libxl_bitmap_set void libxl_bitmap_set(libxl_bitmap *bitmap, int bit) { if (bit >= bitmap->size * 8) return; bitmap->map[bit / 8] |= 1 << (bit & 7); } I referenced above code. It just ignore the cpu number beyond the max CPU count rather than raise an error. thanks zduan
On 2016/4/12 11:35, Zhenzhong Duan wrote: > On 2016/4/11 19:27, Wei Liu wrote: >> On Mon, Apr 11, 2016 at 09:42:57AM +0800, Zhenzhong Duan wrote: >>> It's tool's duty to pass a correct cpumap to XEN. On a host with >>> less than >>> 64 >>> CPUS, it just shows below error. >>> >>> [root@localhost /]# xm vcpu-pin 3 all all >>> Error: Cannot pin vcpu: 0 to cpu: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, >>> 11, 12, >>> 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, >>> 30, 31, >>> 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, >>> 49, 50, >>> 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63] - (22, 'Invalid >>> argument') >>> >> This error clearly shows that the upper layer is not passing down the >> right cpumap. So this example along won't convince me to that this is a >> bug in python binding. It's free to reject input that it deems wrong. >> >> I guess the question is more or less what should it do in such >> situation. So ... >> >>> The fix make it same as in xl code. >>> >> Is there reference to this? I guess the commit message or mail thread >> will shed more light on the expected behaviour. > Calling path: > main_vcpupin->cpurange_parse->update_cpumap_range->libxl_bitmap_set > > void libxl_bitmap_set(libxl_bitmap *bitmap, int bit) > { > if (bit >= bitmap->size * 8) > return; > bitmap->map[bit / 8] |= 1 << (bit & 7); > } > > I referenced above code. It just ignore the cpu number beyond the max > CPU count rather than raise an error. Any further comments? thanks zduan
Sorry I was at the Xen hackathon in the last two days. On Tue, Apr 12, 2016 at 11:35:20AM +0800, Zhenzhong Duan wrote: > ? 2016/4/11 19:27, Wei Liu ??: > >On Mon, Apr 11, 2016 at 09:42:57AM +0800, Zhenzhong Duan wrote: > >>It's tool's duty to pass a correct cpumap to XEN. On a host with less than > >>64 > >>CPUS, it just shows below error. > >> > >>[root@localhost /]# xm vcpu-pin 3 all all > >>Error: Cannot pin vcpu: 0 to cpu: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, > >>13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, > >>32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, > >>51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63] - (22, 'Invalid > >>argument') > >> > >This error clearly shows that the upper layer is not passing down the > >right cpumap. So this example along won't convince me to that this is a > >bug in python binding. It's free to reject input that it deems wrong. > > > >I guess the question is more or less what should it do in such > >situation. So ... > > > >>The fix make it same as in xl code. > >> > >Is there reference to this? I guess the commit message or mail thread > >will shed more light on the expected behaviour. > Calling path: > main_vcpupin->cpurange_parse->update_cpumap_range->libxl_bitmap_set > > void libxl_bitmap_set(libxl_bitmap *bitmap, int bit) > { > if (bit >= bitmap->size * 8) > return; > bitmap->map[bit / 8] |= 1 << (bit & 7); > } > > I referenced above code. It just ignore the cpu number beyond the max CPU > count rather than raise an error. > In principle I think having python binding and xl/libxl behave more or less the same is the right direction. I'm a bit nervous about the change of behaviour on the other hand. Let's wait for a few more days to see if other people have any comment on this. Wei. > thanks > zduan
On Wed, Apr 20, 2016 at 03:33:13PM +0100, Wei Liu wrote: > Sorry I was at the Xen hackathon in the last two days. > > On Tue, Apr 12, 2016 at 11:35:20AM +0800, Zhenzhong Duan wrote: > > ? 2016/4/11 19:27, Wei Liu ??: > > >On Mon, Apr 11, 2016 at 09:42:57AM +0800, Zhenzhong Duan wrote: > > >>It's tool's duty to pass a correct cpumap to XEN. On a host with less than > > >>64 > > >>CPUS, it just shows below error. > > >> > > >>[root@localhost /]# xm vcpu-pin 3 all all > > >>Error: Cannot pin vcpu: 0 to cpu: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, > > >>13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, > > >>32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, > > >>51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63] - (22, 'Invalid > > >>argument') > > >> > > >This error clearly shows that the upper layer is not passing down the > > >right cpumap. So this example along won't convince me to that this is a > > >bug in python binding. It's free to reject input that it deems wrong. > > > > > >I guess the question is more or less what should it do in such > > >situation. So ... > > > > > >>The fix make it same as in xl code. > > >> > > >Is there reference to this? I guess the commit message or mail thread > > >will shed more light on the expected behaviour. > > Calling path: > > main_vcpupin->cpurange_parse->update_cpumap_range->libxl_bitmap_set > > > > void libxl_bitmap_set(libxl_bitmap *bitmap, int bit) > > { > > if (bit >= bitmap->size * 8) > > return; > > bitmap->map[bit / 8] |= 1 << (bit & 7); > > } > > > > I referenced above code. It just ignore the cpu number beyond the max CPU > > count rather than raise an error. > > > > In principle I think having python binding and xl/libxl behave more or less > the same is the right direction. I'm a bit nervous about the change of > behaviour on the other hand. > > Let's wait for a few more days to see if other people have any comment on > this. Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> ? > > Wei. > > > thanks > > zduan > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH] Fix cpumap setting before passing to XEN"):
> On Wed, Apr 20, 2016 at 03:33:13PM +0100, Wei Liu wrote:
> > In principle I think having python binding and xl/libxl behave more or less
> > the same is the right direction. I'm a bit nervous about the change of
> > behaviour on the other hand.
> >
> > Let's wait for a few more days to see if other people have any comment on
> > this.
>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> ?
Does this bug report mean that `xm vcpu-pin ... all' has never
worked properly ? Can that really be the case ?
Also, xm exists in Xen 4.4 and earlier, only. Xen 4.4 is no longer
supported upstream, so we would not apply this patch to Xen 4.4. So
whatever we do, this is not going to fix any bug in `xm vcpu-pin' in
4.4.
This doesn't necessarily mean that I object to changing the behaviour
of the python xc module in still-supported Xen releases. But I'm not
sure the reasoning behind the behaviour of the libxl bitmap functions
applies to the Python interface.
Zhenzhong Duan, are you using an out-of-tree copy of xm and xend ?
Regards,
Ian.
On 2016/4/25 21:26, Ian Jackson wrote: > Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH] Fix cpumap setting before passing to XEN"): >> On Wed, Apr 20, 2016 at 03:33:13PM +0100, Wei Liu wrote: >>> In principle I think having python binding and xl/libxl behave more or less >>> the same is the right direction. I'm a bit nervous about the change of >>> behaviour on the other hand. >>> >>> Let's wait for a few more days to see if other people have any comment on >>> this. >> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> ? > Does this bug report mean that `xm vcpu-pin ... all' has never > worked properly ? Can that really be the case ? Xen 4.3 doesn't work, Xen 3.4 works. I have no Xen 4.4 around to test that, but checked code, it will not. Then I found below commit involved. commit 41abbadef60e5fccdfd688579dd458f7f7887cf5 Author: Ian Jackson <ian.jackson@eu.citrix.com> Date: Wed May 29 15:48:11 2013 +0100 libxc: limit cpu values when setting vcpu affinity When support for pinning more than 64 cpus was added, check for cpu out-of-range values was removed. This can lead to subsequent out-of-bounds cpumap array accesses in case the cpu number is higher than the actual count. This patch returns the check. This is CVE-2013-2072 / XSA-56 Signed-off-by: Petr Matousek <pmatouse@redhat.com> > > Also, xm exists in Xen 4.4 and earlier, only. Xen 4.4 is no longer > supported upstream, so we would not apply this patch to Xen 4.4. So > whatever we do, this is not going to fix any bug in `xm vcpu-pin' in > 4.4. The only impact is upper layer or the user need to pass a correct cpumap param not beyond the real cpu map to avoid the error. But I am not clear if python binding is still used or will be removed just as Xend. > > This doesn't necessarily mean that I object to changing the behaviour > of the python xc module in still-supported Xen releases. But I'm not > sure the reasoning behind the behaviour of the libxl bitmap functions > applies to the Python interface. > > Zhenzhong Duan, are you using an out-of-tree copy of xm and xend ? I am using xen-4.3.0-55.el6.47.33 which is Xen 4.3 variant thanks zduan
On Tue, Apr 26, 2016 at 11:54:32AM +0800, Zhenzhong Duan wrote: > On 2016/4/25 21:26, Ian Jackson wrote: > >Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH] Fix cpumap setting before passing to XEN"): > >>On Wed, Apr 20, 2016 at 03:33:13PM +0100, Wei Liu wrote: > >>>In principle I think having python binding and xl/libxl behave more or less > >>>the same is the right direction. I'm a bit nervous about the change of > >>>behaviour on the other hand. > >>> > >>>Let's wait for a few more days to see if other people have any comment on > >>>this. > >>Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> ? > >Does this bug report mean that `xm vcpu-pin ... all' has never > >worked properly ? Can that really be the case ? > Xen 4.3 doesn't work, Xen 3.4 works. > I have no Xen 4.4 around to test that, but checked code, it will not. > Then I found below commit involved. > > commit 41abbadef60e5fccdfd688579dd458f7f7887cf5 > Author: Ian Jackson <ian.jackson@eu.citrix.com> > Date: Wed May 29 15:48:11 2013 +0100 > > libxc: limit cpu values when setting vcpu affinity > > When support for pinning more than 64 cpus was added, check for cpu > out-of-range values was removed. This can lead to subsequent > out-of-bounds cpumap array accesses in case the cpu number is higher > than the actual count. > > This patch returns the check. > > This is CVE-2013-2072 / XSA-56 > > Signed-off-by: Petr Matousek <pmatouse@redhat.com> > > > >Also, xm exists in Xen 4.4 and earlier, only. Xen 4.4 is no longer > >supported upstream, so we would not apply this patch to Xen 4.4. So > >whatever we do, this is not going to fix any bug in `xm vcpu-pin' in > >4.4. > The only impact is upper layer or the user need to pass a correct cpumap > param not beyond the real cpu map to avoid the error. > But I am not clear if python binding is still used or will be removed just > as Xend. I don't think we have plan to remove it any time soon. On the other hand because no in-tree component uses it so we don't know whether it works in practice or not. > > > >This doesn't necessarily mean that I object to changing the behaviour > >of the python xc module in still-supported Xen releases. But I'm not > >sure the reasoning behind the behaviour of the libxl bitmap functions > >applies to the Python interface. > > > >Zhenzhong Duan, are you using an out-of-tree copy of xm and xend ? > I am using xen-4.3.0-55.el6.47.33 which is Xen 4.3 variant > So what is the conclusion of this discussion so far? I admit I'm a bit lost here. Wei. > thanks > zduan
Wei Liu writes ("Re: [Xen-devel] [PATCH] Fix cpumap setting before passing to XEN"):
> So what is the conclusion of this discussion so far? I admit I'm a bit
> lost here.
Undoubtedly:
* That "xm ..." generates the reported error is definitely a bug.
* "xm" exists only in versions of Xen now no longer supported
upstream.
* Applying the proposed patch to libxc in upstream supported versions
of Xen will not fix "xm" for users like Zhenzhong Duan.
There are two possible views about the nature of the bug:
1. It is xm's fault for passing an invalid cpumap.
2. It is python xc's fault for failing to tolerate a cpumap with
bits set which do not correspond to actual cpus.
In the case 1., the xm python code needs to be changed. But there is
nothing for upstream to do because none of our supported trees contain
this code any more.
In the case 2., the in-tree python xc code should be changed.
I am somewhat reluctant to go down the path implied by case 2, because
we don't know what collateral damage there may be. OTOH the proposed
patch is one which tolerates a wider range of inputs, which is fairly
safe.
I think that the behaviour of libxl (which has to provide a C API,
rather than a python one) is a poor guide to the best behaviour for
python xc.
So I still don't have a clear view about whether the patch should be
applied. (I think it clearly shouldn't be backported.)
Ian.
On Thu, Apr 28, 2016 at 06:02:54PM +0100, Ian Jackson wrote: > Wei Liu writes ("Re: [Xen-devel] [PATCH] Fix cpumap setting before passing to XEN"): > > So what is the conclusion of this discussion so far? I admit I'm a bit > > lost here. > > Undoubtedly: > > * That "xm ..." generates the reported error is definitely a bug. > > * "xm" exists only in versions of Xen now no longer supported > upstream. > > * Applying the proposed patch to libxc in upstream supported versions > of Xen will not fix "xm" for users like Zhenzhong Duan. Wait, how will it not? It will continue without erroring out. > > > There are two possible views about the nature of the bug: > > 1. It is xm's fault for passing an invalid cpumap. > > 2. It is python xc's fault for failing to tolerate a cpumap with > bits set which do not correspond to actual cpus. > > In the case 1., the xm python code needs to be changed. But there is > nothing for upstream to do because none of our supported trees contain > this code any more. > > In the case 2., the in-tree python xc code should be changed. > > > I am somewhat reluctant to go down the path implied by case 2, because > we don't know what collateral damage there may be. OTOH the proposed > patch is one which tolerates a wider range of inputs, which is fairly > safe. > > I think that the behaviour of libxl (which has to provide a C API, > rather than a python one) is a poor guide to the best behaviour for > python xc. > > > So I still don't have a clear view about whether the patch should be > applied. (I think it clearly shouldn't be backported.) I concur on backporting - but I think having it upstream will save the folks who bind to the Python libxc code and eventually will hit this. > > Ian.
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c index c40a4e9..2f4898d 100644 --- a/tools/python/xen/lowlevel/xc/xc.c +++ b/tools/python/xen/lowlevel/xc/xc.c @@ -243,13 +243,15 @@ static PyObject *pyxc_vcpu_setaffinity(XcObject *self, for ( i = 0; i < PyList_Size(cpulist); i++ ) { long cpu = PyInt_AsLong(PyList_GetItem(cpulist, i)); - if ( cpu < 0 || cpu >= nr_cpus ) + if ( cpu < 0 ) { free(cpumap); errno = EINVAL; PyErr_SetFromErrno(xc_error_obj); return NULL; } + if ( cpu >= nr_cpus ) + continue; cpumap[cpu / 8] |= 1 << (cpu % 8); } }
It's tool's duty to pass a correct cpumap to XEN. On a host with less than 64 CPUS, it just shows below error. [root@localhost /]# xm vcpu-pin 3 all all Error: Cannot pin vcpu: 0 to cpu: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63] - (22, 'Invalid argument') The fix make it same as in xl code. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com> --- tools/python/xen/lowlevel/xc/xc.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)