Eamon Walsh
2011-Mar-03 23:29 UTC
[Xen-devel] [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
The function flexarray_vappend() will stop at the first NULL
argument. In libxl_device_vfb_add(), this has been observed
to result in keys being added to the backend array without
associated values in cases where the value can be NULL.
This causes libxl__xs_kvs_of_flexarray(), which expects an even
number of array elements, to write past the end of the array.
Fix by manually appending two values.
Signed-off-by: Eamon Walsh <ewalsh@tycho.nsa.gov>
---
diff -r c5d121fd35c0 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Mon Feb 28 16:55:20 2011 +0000
+++ b/tools/libxl/libxl.c Thu Mar 03 18:32:10 2011 -0500
@@ -1890,9 +1890,11 @@
flexarray_vappend(back, "frontend-id", libxl__sprintf(&gc,
"%d", vfb->domid), NULL);
flexarray_vappend(back, "online", "1", NULL);
flexarray_vappend(back, "state", libxl__sprintf(&gc,
"%d", 1), NULL);
- flexarray_vappend(back, "domain", libxl__domid_to_name(&gc,
domid), NULL);
+ flexarray_append(back, "domain");
+ flexarray_append(back, libxl__domid_to_name(&gc, domid));
flexarray_vappend(back, "vnc", libxl__sprintf(&gc,
"%d", vfb->vnc), NULL);
- flexarray_vappend(back, "vnclisten", vfb->vnclisten, NULL);
+ flexarray_append(back, "vnclisten");
+ flexarray_append(back, vfb->vnclisten);
flexarray_append(back, "vncpasswd");
flexarray_append(back, vfb->vncpasswd);
flexarray_vappend(back, "vncdisplay", libxl__sprintf(&gc,
"%d", vfb->vncdisplay), NULL);
--
Eamon Walsh
National Security Agency
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-04 10:02 UTC
Re: [Xen-devel] [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote:> The function flexarray_vappend() will stop at the first NULL > argument. In libxl_device_vfb_add(), this has been observed > to result in keys being added to the backend array without > associated values in cases where the value can be NULL.If these values are NULL should we be writing them at all? e.g. for: flexarray_vappend(back, foo, bar); where bar may be NULL shouldn''t it become: if (bar) flexarray_vappend(back, foo, bar); or perhaps: flexarray_vappend(back, foo, bar ? bar : ""); ?> This causes libxl__xs_kvs_of_flexarray(), which expects an even > number of array elements, to write past the end of the array.Sounds like libxl__xs_kvs_of_flexarray could also be made more robust here... Ian.> > Fix by manually appending two values. > > Signed-off-by: Eamon Walsh <ewalsh@tycho.nsa.gov> > --- > > diff -r c5d121fd35c0 tools/libxl/libxl.c > --- a/tools/libxl/libxl.c Mon Feb 28 16:55:20 2011 +0000 > +++ b/tools/libxl/libxl.c Thu Mar 03 18:32:10 2011 -0500 > @@ -1890,9 +1890,11 @@ > flexarray_vappend(back, "frontend-id", libxl__sprintf(&gc, "%d", vfb->domid), NULL); > flexarray_vappend(back, "online", "1", NULL); > flexarray_vappend(back, "state", libxl__sprintf(&gc, "%d", 1), NULL); > - flexarray_vappend(back, "domain", libxl__domid_to_name(&gc, domid), NULL); > + flexarray_append(back, "domain"); > + flexarray_append(back, libxl__domid_to_name(&gc, domid)); > flexarray_vappend(back, "vnc", libxl__sprintf(&gc, "%d", vfb->vnc), NULL); > - flexarray_vappend(back, "vnclisten", vfb->vnclisten, NULL); > + flexarray_append(back, "vnclisten"); > + flexarray_append(back, vfb->vnclisten); > flexarray_append(back, "vncpasswd"); > flexarray_append(back, vfb->vncpasswd); > flexarray_vappend(back, "vncdisplay", libxl__sprintf(&gc, "%d", vfb->vncdisplay), NULL); > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Mar-07 13:12 UTC
Re: [Xen-devel] [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
On Fri, 4 Mar 2011, Ian Campbell wrote:> On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote: > > The function flexarray_vappend() will stop at the first NULL > > argument. In libxl_device_vfb_add(), this has been observed > > to result in keys being added to the backend array without > > associated values in cases where the value can be NULL. > > If these values are NULL should we be writing them at all? e.g. for: > flexarray_vappend(back, foo, bar); > where bar may be NULL shouldn''t it become: > if (bar) > flexarray_vappend(back, foo, bar); > or perhaps: > flexarray_vappend(back, foo, bar ? bar : ""); > ? >This is actually a serious issue because it means that every time flexarray_vappend is used and the argument is NULL the behaviour is going to be different from what the coder expected. Maybe flexarray_vappend should assume that the number of args is odd and greater than 2? At least in that case flexarray_vappend would only break if the user misused the function. Or we could use a terminator other than NULL... _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-07 14:13 UTC
Re: [Xen-devel] [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
On Mon, 2011-03-07 at 13:12 +0000, Stefano Stabellini wrote:> On Fri, 4 Mar 2011, Ian Campbell wrote: > > On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote: > > > The function flexarray_vappend() will stop at the first NULL > > > argument. In libxl_device_vfb_add(), this has been observed > > > to result in keys being added to the backend array without > > > associated values in cases where the value can be NULL. > > > > If these values are NULL should we be writing them at all? e.g. for: > > flexarray_vappend(back, foo, bar); > > where bar may be NULL shouldn''t it become: > > if (bar) > > flexarray_vappend(back, foo, bar); > > or perhaps: > > flexarray_vappend(back, foo, bar ? bar : ""); > > ? > > > > This is actually a serious issue because it means that every time > flexarray_vappend is used and the argument is NULL the behaviour is > going to be different from what the coder expected. > Maybe flexarray_vappend should assume that the number of args is odd and > greater than 2?flexarray_vappend is not solely used with libxl__xs_kvs_of_flexarray though, in other cases it may be perfectly valid to have an odd number of items. flexarray_vappend_pair() perhaps? Ian.> At least in that case flexarray_vappend would only break if the user > misused the function. > Or we could use a terminator other than NULL..._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Mar-07 15:04 UTC
Re: [Xen-devel] [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
On Mon, 7 Mar 2011, Ian Campbell wrote:> On Mon, 2011-03-07 at 13:12 +0000, Stefano Stabellini wrote: > > On Fri, 4 Mar 2011, Ian Campbell wrote: > > > On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote: > > > > The function flexarray_vappend() will stop at the first NULL > > > > argument. In libxl_device_vfb_add(), this has been observed > > > > to result in keys being added to the backend array without > > > > associated values in cases where the value can be NULL. > > > > > > If these values are NULL should we be writing them at all? e.g. for: > > > flexarray_vappend(back, foo, bar); > > > where bar may be NULL shouldn''t it become: > > > if (bar) > > > flexarray_vappend(back, foo, bar); > > > or perhaps: > > > flexarray_vappend(back, foo, bar ? bar : ""); > > > ? > > > > > > > This is actually a serious issue because it means that every time > > flexarray_vappend is used and the argument is NULL the behaviour is > > going to be different from what the coder expected. > > Maybe flexarray_vappend should assume that the number of args is odd and > > greater than 2? > > flexarray_vappend is not solely used with libxl__xs_kvs_of_flexarray > though, in other cases it may be perfectly valid to have an odd number > of items. > > flexarray_vappend_pair() perhaps?flexarray_vappend_pairs considering that it is going to take a variable number of pairs? Otherwise we might as well have a flexarray_append_pair that doesn''t use vargs at all. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-07 15:07 UTC
Re: [Xen-devel] [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
On Mon, 2011-03-07 at 15:04 +0000, Stefano Stabellini wrote:> On Mon, 7 Mar 2011, Ian Campbell wrote: > > On Mon, 2011-03-07 at 13:12 +0000, Stefano Stabellini wrote: > > > On Fri, 4 Mar 2011, Ian Campbell wrote: > > > > On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote: > > > > > The function flexarray_vappend() will stop at the first NULL > > > > > argument. In libxl_device_vfb_add(), this has been observed > > > > > to result in keys being added to the backend array without > > > > > associated values in cases where the value can be NULL. > > > > > > > > If these values are NULL should we be writing them at all? e.g. for: > > > > flexarray_vappend(back, foo, bar); > > > > where bar may be NULL shouldn''t it become: > > > > if (bar) > > > > flexarray_vappend(back, foo, bar); > > > > or perhaps: > > > > flexarray_vappend(back, foo, bar ? bar : ""); > > > > ? > > > > > > > > > > This is actually a serious issue because it means that every time > > > flexarray_vappend is used and the argument is NULL the behaviour is > > > going to be different from what the coder expected. > > > Maybe flexarray_vappend should assume that the number of args is odd and > > > greater than 2? > > > > flexarray_vappend is not solely used with libxl__xs_kvs_of_flexarray > > though, in other cases it may be perfectly valid to have an odd number > > of items. > > > > flexarray_vappend_pair() perhaps? > > flexarray_vappend_pairs considering that it is going to take a variable > number of pairs?That was what I was thinking at the time...> Otherwise we might as well have a flexarray_append_pair that doesn''t use > vargs at all.... but I think this makes more sense. Anywhere which actually does use vappend to append multiple pairs would probably be clearer as a series of these anyway. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Mar-07 15:08 UTC
Re: [Xen-devel] [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
On Mon, 7 Mar 2011, Ian Campbell wrote:> > flexarray_vappend_pairs considering that it is going to take a variable > > number of pairs? > > That was what I was thinking at the time... > > > Otherwise we might as well have a flexarray_append_pair that doesn''t use > > vargs at all. > > ... but I think this makes more sense. Anywhere which actually does use > vappend to append multiple pairs would probably be clearer as a series > of these anyway.I agree _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Eamon Walsh
2011-Mar-07 15:33 UTC
Re: [Xen-devel] [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
On 03/04/2011 05:02 AM, Ian Campbell wrote:> On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote: >> The function flexarray_vappend() will stop at the first NULL >> argument. In libxl_device_vfb_add(), this has been observed >> to result in keys being added to the backend array without >> associated values in cases where the value can be NULL. > If these values are NULL should we be writing them at all? e.g. for: > flexarray_vappend(back, foo, bar); > where bar may be NULL shouldn''t it become: > if (bar) > flexarray_vappend(back, foo, bar); > or perhaps: > flexarray_vappend(back, foo, bar ? bar : ""); > ? >If the value is NULL, the key is skipped and not written. This is because of a patch I submitted to change the xs_writev() function, which was calling strlen(NULL) previously. See: http://lists.xensource.com/archives/html/xen-devel/2010-03/msg00703.html However this behavior is not obvious. Checking the value earlier and leaving it off the list makes sense. -- Eamon Walsh National Security Agency _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Mar-07 16:44 UTC
Re: [Xen-devel] [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
On Mon, 7 Mar 2011, Eamon Walsh wrote:> On 03/04/2011 05:02 AM, Ian Campbell wrote: > > On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote: > >> The function flexarray_vappend() will stop at the first NULL > >> argument. In libxl_device_vfb_add(), this has been observed > >> to result in keys being added to the backend array without > >> associated values in cases where the value can be NULL. > > If these values are NULL should we be writing them at all? e.g. for: > > flexarray_vappend(back, foo, bar); > > where bar may be NULL shouldn''t it become: > > if (bar) > > flexarray_vappend(back, foo, bar); > > or perhaps: > > flexarray_vappend(back, foo, bar ? bar : ""); > > ? > > > > If the value is NULL, the key is skipped and not written. This is because of a patch I submitted to change the xs_writev() function, which was calling strlen(NULL) previously. See: > http://lists.xensource.com/archives/html/xen-devel/2010-03/msg00703.html > > However this behavior is not obvious. Checking the value earlier and leaving it off the list makes sense.I think me and Ian reached an agreement on the introduction of flexarray_append_pair that would be very similar to flexarray_append but takes two ptr arguments. flexarray_append_pair would be used instead of flexarray_vappend in libxl_device_vfb_add. What do you think? Would you be up for writing the patch? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel