* [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters
@ 2021-03-05 12:10 Julien Grall
2021-03-05 12:44 ` Jürgen Groß
2021-03-05 13:22 ` Ian Jackson
0 siblings, 2 replies; 11+ messages in thread
From: Julien Grall @ 2021-03-05 12:10 UTC (permalink / raw)
To: xen-devel; +Cc: raphning, iwj, Julien Grall
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters
2021-03-05 12:10 [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters Julien Grall
@ 2021-03-05 12:44 ` Jürgen Groß
2021-03-05 13:22 ` Ian Jackson
1 sibling, 0 replies; 11+ messages in thread
From: Jürgen Groß @ 2021-03-05 12:44 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: raphning, iwj, Julien Grall
[-- Attachment #1.1.1: Type: text/plain, Size: 542 bytes --]
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
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters
2021-03-05 12:10 [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters Julien Grall
2021-03-05 12:44 ` Jürgen Groß
@ 2021-03-05 13:22 ` Ian Jackson
2021-03-05 14:33 ` Jürgen Groß
2021-03-06 19:39 ` Julien Grall
1 sibling, 2 replies; 11+ messages in thread
From: Ian Jackson @ 2021-03-05 13:22 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, raphning, iwj, Julien Grall, Juergen Gross
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters
2021-03-05 13:22 ` Ian Jackson
@ 2021-03-05 14:33 ` Jürgen Groß
2021-03-05 14:37 ` Ian Jackson
2021-03-06 19:39 ` Julien Grall
1 sibling, 1 reply; 11+ messages in thread
From: Jürgen Groß @ 2021-03-05 14:33 UTC (permalink / raw)
To: Ian Jackson, Julien Grall; +Cc: xen-devel, raphning, Julien Grall
[-- Attachment #1.1.1: Type: text/plain, Size: 989 bytes --]
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
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters
2021-03-05 14:33 ` Jürgen Groß
@ 2021-03-05 14:37 ` Ian Jackson
2021-03-05 14:41 ` Jürgen Groß
0 siblings, 1 reply; 11+ messages in thread
From: Ian Jackson @ 2021-03-05 14:37 UTC (permalink / raw)
To: Jürgen Groß; +Cc: Julien Grall, xen-devel, raphning, Julien Grall
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters
2021-03-05 14:37 ` Ian Jackson
@ 2021-03-05 14:41 ` Jürgen Groß
2021-03-05 14:46 ` Ian Jackson
0 siblings, 1 reply; 11+ messages in thread
From: Jürgen Groß @ 2021-03-05 14:41 UTC (permalink / raw)
To: Ian Jackson; +Cc: Julien Grall, xen-devel, raphning, Julien Grall
[-- Attachment #1.1.1: Type: text/plain, Size: 705 bytes --]
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
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters
2021-03-05 14:41 ` Jürgen Groß
@ 2021-03-05 14:46 ` Ian Jackson
2021-03-05 14:49 ` Ian Jackson
2021-03-05 14:56 ` Jürgen Groß
0 siblings, 2 replies; 11+ messages in thread
From: Ian Jackson @ 2021-03-05 14:46 UTC (permalink / raw)
To: Jürgen Groß; +Cc: Julien Grall, xen-devel, raphning, Julien Grall
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters
2021-03-05 14:46 ` Ian Jackson
@ 2021-03-05 14:49 ` Ian Jackson
2021-03-05 14:55 ` Jürgen Groß
2021-03-05 14:56 ` Jürgen Groß
1 sibling, 1 reply; 11+ messages in thread
From: Ian Jackson @ 2021-03-05 14:49 UTC (permalink / raw)
To: Jürgen Groß, Julien Grall, xen-devel, raphning, Julien Grall
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters
2021-03-05 14:49 ` Ian Jackson
@ 2021-03-05 14:55 ` Jürgen Groß
0 siblings, 0 replies; 11+ messages in thread
From: Jürgen Groß @ 2021-03-05 14:55 UTC (permalink / raw)
To: Ian Jackson, Julien Grall, xen-devel, raphning, Julien Grall
[-- Attachment #1.1.1: Type: text/plain, Size: 2056 bytes --]
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
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters
2021-03-05 14:46 ` Ian Jackson
2021-03-05 14:49 ` Ian Jackson
@ 2021-03-05 14:56 ` Jürgen Groß
1 sibling, 0 replies; 11+ messages in thread
From: Jürgen Groß @ 2021-03-05 14:56 UTC (permalink / raw)
To: Ian Jackson; +Cc: Julien Grall, xen-devel, raphning, Julien Grall
[-- Attachment #1.1.1: Type: text/plain, Size: 1529 bytes --]
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
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters
2021-03-05 13:22 ` Ian Jackson
2021-03-05 14:33 ` Jürgen Groß
@ 2021-03-06 19:39 ` Julien Grall
1 sibling, 0 replies; 11+ messages in thread
From: Julien Grall @ 2021-03-06 19:39 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, raphning, Julien Grall, Juergen Gross
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,
--
Julien Grall
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-03-06 19:39 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-05 12:10 [PATCH for-4.15] tools/xenstored: liveupdate: Increase the maximum number of parameters Julien Grall
2021-03-05 12:44 ` Jürgen Groß
2021-03-05 13:22 ` Ian Jackson
2021-03-05 14:33 ` Jürgen Groß
2021-03-05 14:37 ` Ian Jackson
2021-03-05 14:41 ` Jürgen Groß
2021-03-05 14:46 ` Ian Jackson
2021-03-05 14:49 ` Ian Jackson
2021-03-05 14:55 ` Jürgen Groß
2021-03-05 14:56 ` Jürgen Groß
2021-03-06 19:39 ` Julien Grall
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).