Re: [PATCH] xen/hypfs: fix writing of custom parameter

[Jürgen Groß <jgross@xxxxxxxx>]

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@xxxxxxxxxx>
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> This does fix my bug, so Tested-by: Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>
>
> 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