From: Julien Grall <jgrall@amazon.com>
The longest possible command line for LiveUpdate is:
liveupdate -s -t <timeout> -F
This is 5 parameters. However, the maximum is currently specified to 4.
This means the some of the parameters will get ignored.
Update the field max_pars to 5 so and admin can specify the timeout and
force at the same time.
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
This is a candidate for Xen 4.15. Without it, it would not be possible
to pass -F and -t together.
The change is only modifying behavior for XenStored LiveUpdate.
---
tools/xenstore/xenstored_control.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index a1652219b247..8e470f2b2056 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -768,7 +768,7 @@ static struct cmd_s cmds[] = {
*/
{ "live-update", do_control_lu,
"[-c <cmdline>] [-F] [-t <timeout>] <file>\n"
- " Default timeout is 60 seconds.", 4 },
+ " Default timeout is 60 seconds.", 5 },
#endif
#ifdef __MINIOS__
{ "memreport", do_control_memreport, "" },
--
2.17.1
On 05.03.21 13:10, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > The longest possible command line for LiveUpdate is: > > liveupdate -s -t <timeout> -F > > This is 5 parameters. However, the maximum is currently specified to 4. > This means the some of the parameters will get ignored. > > Update the field max_pars to 5 so and admin can specify the timeout and > force at the same time. > > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
Julien Grall writes ("[PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
> From: Julien Grall <jgrall@amazon.com>
>
> The longest possible command line for LiveUpdate is:
>
> liveupdate -s -t <timeout> -F
>
> This is 5 parameters. However, the maximum is currently specified to 4.
> This means the some of the parameters will get ignored.
Why are the extra parameters ignored rather than treated as errors ?
This seems like an invitation to making code with bad behaviour
(perhaps bad security-relevant behaviour).
CC Juergen who seems to have written the code...
> Update the field max_pars to 5 so and admin can specify the timeout and
> force at the same time.
Anyway, for this patch,
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
On 05.03.21 14:22, Ian Jackson wrote:
> Julien Grall writes ("[PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The longest possible command line for LiveUpdate is:
>>
>> liveupdate -s -t <timeout> -F
>>
>> This is 5 parameters. However, the maximum is currently specified to 4.
>> This means the some of the parameters will get ignored.
>
> Why are the extra parameters ignored rather than treated as errors ?
> This seems like an invitation to making code with bad behaviour
> (perhaps bad security-relevant behaviour).
>
> CC Juergen who seems to have written the code...
This is the max number of 0 delimited string parameters. Especially the
stubdom case needs a binary blob (with length, of course) as parameter,
and the number of 0 bytes in this data is just limited by the allowed
payload length.
See the comment in line 111 of xenstored_control.c.
Juergen
Jürgen Groß writes ("Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
> This is the max number of 0 delimited string parameters. Especially the
> stubdom case needs a binary blob (with length, of course) as parameter,
> and the number of 0 bytes in this data is just limited by the allowed
> payload length.
>
> See the comment in line 111 of xenstored_control.c.
AFAICT this "live-update" command is variadic. So why is this
parameter set here it all then ?
Ian.
On 05.03.21 15:37, Ian Jackson wrote:
> Jürgen Groß writes ("Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
>> This is the max number of 0 delimited string parameters. Especially the
>> stubdom case needs a binary blob (with length, of course) as parameter,
>> and the number of 0 bytes in this data is just limited by the allowed
>> payload length.
>>
>> See the comment in line 111 of xenstored_control.c.
>
> AFAICT this "live-update" command is variadic. So why is this
> parameter set here it all then ?
In order to avoid allocating an array for 4000 arguments when there
are only 5 which need to be treated as strings.
Juergen
Jürgen Groß writes ("Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
> On 05.03.21 15:37, Ian Jackson wrote:
> > Jürgen Groß writes ("Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
> >> This is the max number of 0 delimited string parameters. Especially the
> >> stubdom case needs a binary blob (with length, of course) as parameter,
> >> and the number of 0 bytes in this data is just limited by the allowed
> >> payload length.
> >>
> >> See the comment in line 111 of xenstored_control.c.
> >
> > AFAICT this "live-update" command is variadic. So why is this
> > parameter set here it all then ?
>
> In order to avoid allocating an array for 4000 arguments when there
> are only 5 which need to be treated as strings.
So this parameter is doing two jobs: 1. enabling non-variadic commands
to take binary input; 2. preventing variadic commands from allocating
unbounded memory.
The problem with this is that in case 1 exceeding the value is normal
and the final argument is binary data; whereas in case 2 glomming
together the arguments together with zeroes is wrong and potentially
hazrdous.
I suggest we solve problem 2 by imposing a higher fixed (for all
commands, variadic or not) limit (20 or something) which causes errors
when exceeded, rather than silent argument misinterpretation.
Ian.
On 05.03.21 15:46, Ian Jackson wrote:
> Jürgen Groß writes ("Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
>> On 05.03.21 15:37, Ian Jackson wrote:
>>> Jürgen Groß writes ("Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
>>>> This is the max number of 0 delimited string parameters. Especially the
>>>> stubdom case needs a binary blob (with length, of course) as parameter,
>>>> and the number of 0 bytes in this data is just limited by the allowed
>>>> payload length.
>>>>
>>>> See the comment in line 111 of xenstored_control.c.
>>>
>>> AFAICT this "live-update" command is variadic. So why is this
>>> parameter set here it all then ?
>>
>> In order to avoid allocating an array for 4000 arguments when there
>> are only 5 which need to be treated as strings.
>
> So this parameter is doing two jobs: 1. enabling non-variadic commands
> to take binary input; 2. preventing variadic commands from allocating
> unbounded memory.
>
> The problem with this is that in case 1 exceeding the value is normal
> and the final argument is binary data; whereas in case 2 glomming
> together the arguments together with zeroes is wrong and potentially
> hazrdous.
>
> I suggest we solve problem 2 by imposing a higher fixed (for all
> commands, variadic or not) limit (20 or something) which causes errors
> when exceeded, rather than silent argument misinterpretation.
Patches welcome.
Juergen
Ian Jackson writes ("Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
> Jürgen Groß writes ("Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
> > On 05.03.21 15:37, Ian Jackson wrote:
> > > Jürgen Groß writes ("Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
> > >> This is the max number of 0 delimited string parameters. Especially the
> > >> stubdom case needs a binary blob (with length, of course) as parameter,
> > >> and the number of 0 bytes in this data is just limited by the allowed
> > >> payload length.
> > >>
> > >> See the comment in line 111 of xenstored_control.c.
> > >
> > > AFAICT this "live-update" command is variadic. So why is this
> > > parameter set here it all then ?
> >
> > In order to avoid allocating an array for 4000 arguments when there
> > are only 5 which need to be treated as strings.
>
> So this parameter is doing two jobs: 1. enabling non-variadic commands
> to take binary input; 2. preventing variadic commands from allocating
> unbounded memory.
>
> The problem with this is that in case 1 exceeding the value is normal
> and the final argument is binary data; whereas in case 2 glomming
> together the arguments together with zeroes is wrong and potentially
> hazrdous.
>
> I suggest we solve problem 2 by imposing a higher fixed (for all
> commands, variadic or not) limit (20 or something) which causes errors
> when exceeded, rather than silent argument misinterpretation.
Also, this use of max_pars to do job 2 seems very inconsistent. It is
applied only to "live-update".
If it is necessary for "live-update", which is it not necessary for
"check" or whatever ?
Ian.
On 05.03.21 15:49, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
>> Jürgen Groß writes ("Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
>>> On 05.03.21 15:37, Ian Jackson wrote:
>>>> Jürgen Groß writes ("Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters"):
>>>>> This is the max number of 0 delimited string parameters. Especially the
>>>>> stubdom case needs a binary blob (with length, of course) as parameter,
>>>>> and the number of 0 bytes in this data is just limited by the allowed
>>>>> payload length.
>>>>>
>>>>> See the comment in line 111 of xenstored_control.c.
>>>>
>>>> AFAICT this "live-update" command is variadic. So why is this
>>>> parameter set here it all then ?
>>>
>>> In order to avoid allocating an array for 4000 arguments when there
>>> are only 5 which need to be treated as strings.
>>
>> So this parameter is doing two jobs: 1. enabling non-variadic commands
>> to take binary input; 2. preventing variadic commands from allocating
>> unbounded memory.
>>
>> The problem with this is that in case 1 exceeding the value is normal
>> and the final argument is binary data; whereas in case 2 glomming
>> together the arguments together with zeroes is wrong and potentially
>> hazrdous.
>>
>> I suggest we solve problem 2 by imposing a higher fixed (for all
>> commands, variadic or not) limit (20 or something) which causes errors
>> when exceeded, rather than silent argument misinterpretation.
>
> Also, this use of max_pars to do job 2 seems very inconsistent. It is
> applied only to "live-update".
>
> If it is necessary for "live-update", which is it not necessary for
> "check" or whatever ?
live-update is the only command with binary data. The other commands are
checking all parameters to be valid, similar to normal parameter parsing
in a main() function of a user program.
Juergen
© 2016 - 2024 Red Hat, Inc.