diff mbox series

xen/hypfs: fix writing of custom parameter

Message ID 20200911053043.29445-1-jgross@suse.com
State New
Headers show
Series xen/hypfs: fix writing of custom parameter | expand

Commit Message

Juergen Gross Sept. 11, 2020, 5:30 a.m. UTC
Today the maximum allowed data length for writing a hypfs node is
tested in the generic hypfs_write() function. For custom runtime
parameters this might be wrong, as the maximum allowed size is derived
from the buffer holding the current setting, while there might be ways
to set the parameter needing more characters than the minimal
representation of that value.

One example for this is the "ept" parameter. Its value buffer is sized
to be able to hold the string "exec-sp=0" or "exec-sp=1", while it is
allowed to use e.g. "no-exec-sp" or "exec-sp=yes" for setting it.

Fix that by moving the length check one level down to the type
specific write function.

In order to avoid allocation of arbitrary sized buffers use a new
MAX_PARAM_SIZE macro as an upper limit for custom writes. The value
of MAX_PARAM_SIZE is the same as the limit in parse_params() for a
single parameter.

Fixes: 5b5ccafb0c42 ("xen: add basic hypervisor filesystem support")
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/hypfs.c      | 11 +++++++----
 xen/common/kernel.c     |  2 +-
 xen/include/xen/param.h |  3 +++
 3 files changed, 11 insertions(+), 5 deletions(-)

Comments

Jan Beulich Sept. 11, 2020, 9:31 a.m. UTC | #1
On 11.09.2020 07:30, Juergen Gross wrote:
> Today the maximum allowed data length for writing a hypfs node is
> tested in the generic hypfs_write() function. For custom runtime
> parameters this might be wrong, as the maximum allowed size is derived
> from the buffer holding the current setting, while there might be ways
> to set the parameter needing more characters than the minimal
> representation of that value.
> 
> One example for this is the "ept" parameter. Its value buffer is sized
> to be able to hold the string "exec-sp=0" or "exec-sp=1", while it is
> allowed to use e.g. "no-exec-sp" or "exec-sp=yes" for setting it.
> 
> Fix that by moving the length check one level down to the type
> specific write function.
> 
> In order to avoid allocation of arbitrary sized buffers use a new
> MAX_PARAM_SIZE macro as an upper limit for custom writes. The value
> of MAX_PARAM_SIZE is the same as the limit in parse_params() for a
> single parameter.
> 
> Fixes: 5b5ccafb0c42 ("xen: add basic hypervisor filesystem support")

Perhaps rather a659d7cab9af ("xen: add runtime parameter access
support to hypfs"), as that where hypfs_write_custom() got introduced?

> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Andrew Cooper Sept. 11, 2020, 12:14 p.m. UTC | #2
On 11/09/2020 06:30, Juergen Gross wrote:
> Today the maximum allowed data length for writing a hypfs node is
> tested in the generic hypfs_write() function. For custom runtime
> parameters this might be wrong, as the maximum allowed size is derived
> from the buffer holding the current setting, while there might be ways
> to set the parameter needing more characters than the minimal
> representation of that value.
>
> One example for this is the "ept" parameter. Its value buffer is sized
> to be able to hold the string "exec-sp=0" or "exec-sp=1", while it is
> allowed to use e.g. "no-exec-sp" or "exec-sp=yes" for setting it.

If you're looking for silly examples, exec-sp=disabled is also legal
boolean notation for Xen.

>
> Fix that by moving the length check one level down to the type
> specific write function.
>
> In order to avoid allocation of arbitrary sized buffers use a new
> MAX_PARAM_SIZE macro as an upper limit for custom writes. The value
> of MAX_PARAM_SIZE is the same as the limit in parse_params() for a
> single parameter.
>
> Fixes: 5b5ccafb0c42 ("xen: add basic hypervisor filesystem support")
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>

This does fix my bug, so Tested-by: Andrew Cooper
<andrew.cooper3@citrix.com>

This does need backporting to 4.14, so maybe is best to take in this
form for now.

However, I'm rather uneasy about the approach.

Everything here derives from command line semantics, and it's probably
not going to be long until we get runtime modification of two sub
parameters.

In a theoretical world where all the EPT suboptions were runtime
modifiable, it would be legal to try and pass

ept=exec-sp,pml,no-pml,no-ad,ad,no-ad

While being fairly nonsensical, it is well formed on the command line. 
We go left to right, and latest takes precedence.

Given that we do have buffer containing everything provided by
userspace, and all internal logic organised with const char *, why do we
need an intermediate allocation at all?

Can't we just pass that all the way down, rather than leaving the same
bug to hit at some point when we do have a parameter which legitimately
takes 128 characters of configuration?

~Andrew
Juergen Gross Sept. 11, 2020, 12:28 p.m. UTC | #3
On 11.09.20 14:14, Andrew Cooper wrote:
> On 11/09/2020 06:30, Juergen Gross wrote:
>> Today the maximum allowed data length for writing a hypfs node is
>> tested in the generic hypfs_write() function. For custom runtime
>> parameters this might be wrong, as the maximum allowed size is derived
>> from the buffer holding the current setting, while there might be ways
>> to set the parameter needing more characters than the minimal
>> representation of that value.
>>
>> One example for this is the "ept" parameter. Its value buffer is sized
>> to be able to hold the string "exec-sp=0" or "exec-sp=1", while it is
>> allowed to use e.g. "no-exec-sp" or "exec-sp=yes" for setting it.
> 
> If you're looking for silly examples, exec-sp=disabled is also legal
> boolean notation for Xen.
> 
>>
>> Fix that by moving the length check one level down to the type
>> specific write function.
>>
>> In order to avoid allocation of arbitrary sized buffers use a new
>> MAX_PARAM_SIZE macro as an upper limit for custom writes. The value
>> of MAX_PARAM_SIZE is the same as the limit in parse_params() for a
>> single parameter.
>>
>> Fixes: 5b5ccafb0c42 ("xen: add basic hypervisor filesystem support")
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> This does fix my bug, so Tested-by: Andrew Cooper
> <andrew.cooper3@citrix.com>
> 
> This does need backporting to 4.14, so maybe is best to take in this
> form for now.
> 
> However, I'm rather uneasy about the approach.
> 
> Everything here derives from command line semantics, and it's probably
> not going to be long until we get runtime modification of two sub
> parameters.
> 
> In a theoretical world where all the EPT suboptions were runtime
> modifiable, it would be legal to try and pass
> 
> ept=exec-sp,pml,no-pml,no-ad,ad,no-ad

Correct.

> While being fairly nonsensical, it is well formed on the command line.
> We go left to right, and latest takes precedence.

Yes.

> Given that we do have buffer containing everything provided by
> userspace, and all internal logic organised with const char *, why do we
> need an intermediate allocation at all?

Which intermediate allocation?

> Can't we just pass that all the way down, rather than leaving the same
> bug to hit at some point when we do have a parameter which legitimately
> takes 128 characters of configuration?

The problem is we can't just set the current value with the string
passed in from the user.

Imagine above example with ept, just two calls with:

ept=exec-sp
ept=no-pml

Your idea is to return only no-pml, while the truth would be
exec-sp=1,pml=0 (in the notation produced by the current code).

You need to create a correct value based on all valid sub-option
values currently active. And the static buffer for this value is
sized to be able to hold the largest possible value. The only
problem at hand was that the input string could be larger.

And as all parameters today already share the restriction of 128
characters (at least when entered as boot parameters) I don't see
a major problem with this approach.

What I could do easily is to limit the length to:

max(MAX_PARAM_SIZE, sizeof(static_buffer))

in order to allow special runtime-only custom parameters with
larger values.


Juergen
Andrew Cooper Sept. 11, 2020, 2:02 p.m. UTC | #4
On 11/09/2020 13:28, Jürgen Groß wrote:
> On 11.09.20 14:14, Andrew Cooper wrote:
>> On 11/09/2020 06:30, Juergen Gross wrote:
>>> Today the maximum allowed data length for writing a hypfs node is
>>> tested in the generic hypfs_write() function. For custom runtime
>>> parameters this might be wrong, as the maximum allowed size is derived
>>> from the buffer holding the current setting, while there might be ways
>>> to set the parameter needing more characters than the minimal
>>> representation of that value.
>>>
>>> One example for this is the "ept" parameter. Its value buffer is sized
>>> to be able to hold the string "exec-sp=0" or "exec-sp=1", while it is
>>> allowed to use e.g. "no-exec-sp" or "exec-sp=yes" for setting it.
>>
>> If you're looking for silly examples, exec-sp=disabled is also legal
>> boolean notation for Xen.
>>
>>>
>>> Fix that by moving the length check one level down to the type
>>> specific write function.
>>>
>>> In order to avoid allocation of arbitrary sized buffers use a new
>>> MAX_PARAM_SIZE macro as an upper limit for custom writes. The value
>>> of MAX_PARAM_SIZE is the same as the limit in parse_params() for a
>>> single parameter.
>>>
>>> Fixes: 5b5ccafb0c42 ("xen: add basic hypervisor filesystem support")
>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> This does fix my bug, so Tested-by: Andrew Cooper
>> <andrew.cooper3@citrix.com>
>>
>> This does need backporting to 4.14, so maybe is best to take in this
>> form for now.
>>
>> However, I'm rather uneasy about the approach.
>>
>> Everything here derives from command line semantics, and it's probably
>> not going to be long until we get runtime modification of two sub
>> parameters.
>>
>> In a theoretical world where all the EPT suboptions were runtime
>> modifiable, it would be legal to try and pass
>>
>> ept=exec-sp,pml,no-pml,no-ad,ad,no-ad
>
> Correct.
>
>> While being fairly nonsensical, it is well formed on the command line.
>> We go left to right, and latest takes precedence.
>
> Yes.
>
>> Given that we do have buffer containing everything provided by
>> userspace, and all internal logic organised with const char *, why do we
>> need an intermediate allocation at all?
>
> Which intermediate allocation?

Sorry.  Intermediate buffer.

>
>> Can't we just pass that all the way down, rather than leaving the same
>> bug to hit at some point when we do have a parameter which legitimately
>> takes 128 characters of configuration?
>
> The problem is we can't just set the current value with the string
> passed in from the user.

Why ever not?  It is parsed as per the command line, not taken
verbatim.  It has no bearing on size of the output buffer.

>
> Imagine above example with ept, just two calls with:
>
> ept=exec-sp
> ept=no-pml
>
> Your idea is to return only no-pml, while the truth would be
> exec-sp=1,pml=0 (in the notation produced by the current code).

In this example,

The semantic gap is that "xenhypfs cat /params/ept" doesn't mean "tell
me what the user (last?) put on the command line for ept=".  It means
"tell me the current state of the ept= runtime options".

I agree that reading it should always return something of the form
exec-sp=X,pml=Y.

However, writing it should not require the user to provide both in one
go.  Its a horrible (and racy) interface when you only want to change
one of the options.

Specifically, the following should work:

# xenhypfs cat /params/ept
exec-sp=A,pml=B
# xenhypfs write /params/ept exec-sp=C
# xenhypfs cat /params/ept
exec-sp=C,pml=B
# xenhypfs write /params/ept pml=D
# xenhypfs cat /params/ept
exec-sp=C,pml=D

~Andrew

P.S. What stability guarantees have we made about the structure layout? 
Didn't we agree that a top level /params/ wasn't necessarily the best
hierarchy to turn into an ABI.
Juergen Gross Sept. 11, 2020, 2:14 p.m. UTC | #5
On 11.09.20 16:02, Andrew Cooper wrote:
> On 11/09/2020 13:28, Jürgen Groß wrote:
>> On 11.09.20 14:14, Andrew Cooper wrote:
>>> On 11/09/2020 06:30, Juergen Gross wrote:
>>>> Today the maximum allowed data length for writing a hypfs node is
>>>> tested in the generic hypfs_write() function. For custom runtime
>>>> parameters this might be wrong, as the maximum allowed size is derived
>>>> from the buffer holding the current setting, while there might be ways
>>>> to set the parameter needing more characters than the minimal
>>>> representation of that value.
>>>>
>>>> One example for this is the "ept" parameter. Its value buffer is sized
>>>> to be able to hold the string "exec-sp=0" or "exec-sp=1", while it is
>>>> allowed to use e.g. "no-exec-sp" or "exec-sp=yes" for setting it.
>>>
>>> If you're looking for silly examples, exec-sp=disabled is also legal
>>> boolean notation for Xen.
>>>
>>>>
>>>> Fix that by moving the length check one level down to the type
>>>> specific write function.
>>>>
>>>> In order to avoid allocation of arbitrary sized buffers use a new
>>>> MAX_PARAM_SIZE macro as an upper limit for custom writes. The value
>>>> of MAX_PARAM_SIZE is the same as the limit in parse_params() for a
>>>> single parameter.
>>>>
>>>> Fixes: 5b5ccafb0c42 ("xen: add basic hypervisor filesystem support")
>>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> This does fix my bug, so Tested-by: Andrew Cooper
>>> <andrew.cooper3@citrix.com>
>>>
>>> This does need backporting to 4.14, so maybe is best to take in this
>>> form for now.
>>>
>>> However, I'm rather uneasy about the approach.
>>>
>>> Everything here derives from command line semantics, and it's probably
>>> not going to be long until we get runtime modification of two sub
>>> parameters.
>>>
>>> In a theoretical world where all the EPT suboptions were runtime
>>> modifiable, it would be legal to try and pass
>>>
>>> ept=exec-sp,pml,no-pml,no-ad,ad,no-ad
>>
>> Correct.
>>
>>> While being fairly nonsensical, it is well formed on the command line.
>>> We go left to right, and latest takes precedence.
>>
>> Yes.
>>
>>> Given that we do have buffer containing everything provided by
>>> userspace, and all internal logic organised with const char *, why do we
>>> need an intermediate allocation at all?
>>
>> Which intermediate allocation?
> 
> Sorry.  Intermediate buffer.
> 
>>
>>> Can't we just pass that all the way down, rather than leaving the same
>>> bug to hit at some point when we do have a parameter which legitimately
>>> takes 128 characters of configuration?
>>
>> The problem is we can't just set the current value with the string
>> passed in from the user.
> 
> Why ever not?  It is parsed as per the command line, not taken
> verbatim.  It has no bearing on size of the output buffer.

We have a user area copied to the hypervisor buffer. This buffer is then
parsed like the command line and the result is stored in the internal
variables of the hypervisor (e.g. as boolean, int, multiple variables,
what ever).

Then a static buffer is filled with a textual representation reflecting
the internal variable values in order to have a complete picture of the
current setting of the param.

So what do you want to do differently here?

> 
>>
>> Imagine above example with ept, just two calls with:
>>
>> ept=exec-sp
>> ept=no-pml
>>
>> Your idea is to return only no-pml, while the truth would be
>> exec-sp=1,pml=0 (in the notation produced by the current code).
> 
> In this example,
> 
> The semantic gap is that "xenhypfs cat /params/ept" doesn't mean "tell
> me what the user (last?) put on the command line for ept=".  It means
> "tell me the current state of the ept= runtime options".

Right.

> 
> I agree that reading it should always return something of the form
> exec-sp=X,pml=Y.
> 
> However, writing it should not require the user to provide both in one
> go.  Its a horrible (and racy) interface when you only want to change
> one of the options.

Right again.

> 
> Specifically, the following should work:
> 
> # xenhypfs cat /params/ept
> exec-sp=A,pml=B
> # xenhypfs write /params/ept exec-sp=C
> # xenhypfs cat /params/ept
> exec-sp=C,pml=B
> # xenhypfs write /params/ept pml=D
> # xenhypfs cat /params/ept
> exec-sp=C,pml=D

And the current design is to achieve exactly this behavior.

> 
> ~Andrew
> 
> P.S. What stability guarantees have we made about the structure layout?
> Didn't we agree that a top level /params/ wasn't necessarily the best
> hierarchy to turn into an ABI.

I can't remember having seen such a remark.


Juergen
diff mbox series

Patch

diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
index b74c228191..8e932b5cf9 100644
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -297,7 +297,9 @@  int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
     int ret;
 
     ASSERT(this_cpu(hypfs_locked) == hypfs_write_locked);
-    ASSERT(ulen <= leaf->e.max_size);
+
+    if ( ulen > leaf->e.max_size )
+        return -ENOSPC;
 
     if ( leaf->e.type != XEN_HYPFS_TYPE_STRING &&
          leaf->e.type != XEN_HYPFS_TYPE_BLOB && ulen != leaf->e.size )
@@ -356,6 +358,10 @@  int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
 
     ASSERT(this_cpu(hypfs_locked) == hypfs_write_locked);
 
+    /* Avoid oversized buffer allocation. */
+    if ( ulen > MAX_PARAM_SIZE )
+        return -ENOSPC;
+
     buf = xzalloc_array(char, ulen);
     if ( !buf )
         return -ENOMEM;
@@ -386,9 +392,6 @@  static int hypfs_write(struct hypfs_entry *entry,
 
     ASSERT(entry->max_size);
 
-    if ( ulen > entry->max_size )
-        return -ENOSPC;
-
     l = container_of(entry, struct hypfs_entry_leaf, e);
 
     return entry->write(l, uaddr, ulen);
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 9de07b7ac5..c3a943f077 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -57,7 +57,7 @@  static int assign_integer_param(const struct kernel_param *param, uint64_t val)
 static int parse_params(const char *cmdline, const struct kernel_param *start,
                         const struct kernel_param *end)
 {
-    char opt[128], *optval, *optkey, *q;
+    char opt[MAX_PARAM_SIZE], *optval, *optkey, *q;
     const char *p = cmdline, *key;
     const struct kernel_param *param;
     int rc, final_rc = 0;
diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h
index f4be944248..d0409d3a0e 100644
--- a/xen/include/xen/param.h
+++ b/xen/include/xen/param.h
@@ -26,6 +26,9 @@  struct kernel_param {
     } par;
 };
 
+/* Maximum length of a single parameter string. */
+#define MAX_PARAM_SIZE 128
+
 extern const struct kernel_param __setup_start[], __setup_end[];
 
 #define __param(att)      static const att \