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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@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?

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.