* [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
@ 2011-03-03 23:29 Eamon Walsh
2011-03-04 10:02 ` Ian Campbell
0 siblings, 1 reply; 9+ messages in thread
From: Eamon Walsh @ 2011-03-03 23:29 UTC (permalink / raw)
To: Xen-devel; +Cc: stefano.stabellini
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
2011-03-03 23:29 [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL Eamon Walsh
@ 2011-03-04 10:02 ` Ian Campbell
2011-03-07 13:12 ` Stefano Stabellini
2011-03-07 15:33 ` Eamon Walsh
0 siblings, 2 replies; 9+ messages in thread
From: Ian Campbell @ 2011-03-04 10:02 UTC (permalink / raw)
To: Eamon Walsh; +Cc: Xen-devel, Stefano Stabellini
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);
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
2011-03-04 10:02 ` Ian Campbell
@ 2011-03-07 13:12 ` Stefano Stabellini
2011-03-07 14:13 ` Ian Campbell
2011-03-07 15:33 ` Eamon Walsh
1 sibling, 1 reply; 9+ messages in thread
From: Stefano Stabellini @ 2011-03-07 13:12 UTC (permalink / raw)
To: Ian Campbell; +Cc: Eamon Walsh, Xen-devel, Stefano Stabellini
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...
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
2011-03-07 13:12 ` Stefano Stabellini
@ 2011-03-07 14:13 ` Ian Campbell
2011-03-07 15:04 ` Stefano Stabellini
0 siblings, 1 reply; 9+ messages in thread
From: Ian Campbell @ 2011-03-07 14:13 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Eamon Walsh, Xen-devel
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...
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
2011-03-07 14:13 ` Ian Campbell
@ 2011-03-07 15:04 ` Stefano Stabellini
2011-03-07 15:07 ` Ian Campbell
0 siblings, 1 reply; 9+ messages in thread
From: Stefano Stabellini @ 2011-03-07 15:04 UTC (permalink / raw)
To: Ian Campbell; +Cc: Eamon Walsh, Xen-devel, Stefano Stabellini
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
2011-03-07 15:04 ` Stefano Stabellini
@ 2011-03-07 15:07 ` Ian Campbell
2011-03-07 15:08 ` Stefano Stabellini
0 siblings, 1 reply; 9+ messages in thread
From: Ian Campbell @ 2011-03-07 15:07 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Eamon Walsh, Xen-devel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
2011-03-07 15:07 ` Ian Campbell
@ 2011-03-07 15:08 ` Stefano Stabellini
0 siblings, 0 replies; 9+ messages in thread
From: Stefano Stabellini @ 2011-03-07 15:08 UTC (permalink / raw)
To: Ian Campbell; +Cc: Eamon Walsh, Xen-devel, Stefano Stabellini
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
2011-03-04 10:02 ` Ian Campbell
2011-03-07 13:12 ` Stefano Stabellini
@ 2011-03-07 15:33 ` Eamon Walsh
2011-03-07 16:44 ` Stefano Stabellini
1 sibling, 1 reply; 9+ messages in thread
From: Eamon Walsh @ 2011-03-07 15:33 UTC (permalink / raw)
To: Ian Campbell; +Cc: Xen-devel, Stefano Stabellini
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL
2011-03-07 15:33 ` Eamon Walsh
@ 2011-03-07 16:44 ` Stefano Stabellini
0 siblings, 0 replies; 9+ messages in thread
From: Stefano Stabellini @ 2011-03-07 16:44 UTC (permalink / raw)
To: Eamon Walsh; +Cc: Ian Campbell, Xen-devel, Stefano Stabellini
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-03-07 16:44 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-03 23:29 [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL Eamon Walsh
2011-03-04 10:02 ` Ian Campbell
2011-03-07 13:12 ` Stefano Stabellini
2011-03-07 14:13 ` Ian Campbell
2011-03-07 15:04 ` Stefano Stabellini
2011-03-07 15:07 ` Ian Campbell
2011-03-07 15:08 ` Stefano Stabellini
2011-03-07 15:33 ` Eamon Walsh
2011-03-07 16:44 ` Stefano Stabellini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).