diff mbox series

[for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters

Message ID 20210305121029.7047-1-julien@xen.org (mailing list archive)
State New, archived
Headers show
Series [for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters | expand

Commit Message

Julien Grall March 5, 2021, 12:10 p.m. UTC 
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(-)

Comments

Jürgen Groß March 5, 2021, 12:44 p.m. UTC | #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
Ian Jackson March 5, 2021, 1:22 p.m. UTC | #2 
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>
Jürgen Groß March 5, 2021, 2:33 p.m. UTC | #3 
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
Ian Jackson March 5, 2021, 2:37 p.m. UTC | #4 
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.
Jürgen Groß March 5, 2021, 2:41 p.m. UTC | #5 
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
Ian Jackson March 5, 2021, 2:46 p.m. UTC | #6 
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.
Ian Jackson March 5, 2021, 2:49 p.m. UTC | #7 
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.
Jürgen Groß March 5, 2021, 2:55 p.m. UTC | #8 
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
Jürgen Groß March 5, 2021, 2:56 p.m. UTC | #9 
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
Julien Grall March 6, 2021, 7:39 p.m. UTC | #10 
Hi Ian,

On 05/03/2021 13:22, Ian Jackson wrote:
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>

I have committed the patch. Thanks!

Cheers,
diff mbox series

Patch

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, "" },