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 <[email protected]>
>>> Signed-off-by: Juergen Gross <[email protected]>
>>
>> This does fix my bug, so Tested-by: Andrew Cooper
>> <[email protected]>
>>
>> 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.
