[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/hypfs: fix writing of custom parameter
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@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-adCorrect.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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |