Re: [Qemu-devel] [Qemu-block] [PATCH] block/nfs: add support for setting debug level

[Peter Lieven]

Am 26.06.2015 um 11:14 schrieb Stefan Hajnoczi:
> On Thu, Jun 25, 2015 at 03:26:46PM +0200, Peter Lieven wrote:
>> Am 25.06.2015 um 15:18 schrieb Stefan Hajnoczi:
>>> On Tue, Jun 23, 2015 at 10:12:15AM +0200, Peter Lieven wrote:
>>>> upcoming libnfs versions will support logging debug messages. Add
>>>> support for it in qemu through an URL parameter.
>>>>
>>>> Signed-off-by: Peter Lieven <address@hidden>
>>>> ---
>>>>  block/nfs.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/block/nfs.c b/block/nfs.c
>>>> index ca9e24e..f7388a3 100644
>>>> --- a/block/nfs.c
>>>> +++ b/block/nfs.c
>>>> @@ -329,6 +329,10 @@ static int64_t nfs_client_open(NFSClient *client, 
>>>> const char *filename,
>>>>          } else if (!strcmp(qp->p[i].name, "readahead")) {
>>>>              nfs_set_readahead(client->context, val);
>>>>  #endif
>>>> +#ifdef LIBNFS_FEATURE_DEBUG
>>>> +        } else if (!strcmp(qp->p[i].name, "debug")) {
>>>> +            nfs_set_debug(client->context, val);
>>>> +#endif
>>>>          } else {
>>>>              error_setg(errp, "Unknown NFS parameter name: %s",
>>>>                         qp->p[i].name);
>>> Untrusted users may be able to set these options since they are encoded
>>> in the URI.  I'm imagining a hosting or cloud scenario like OpenStack.
>>>
>>> A verbose debug level spams stderr and could consume a lot of disk
>>> space.
>>>
>>> (The uid and gid options are probably okay since the NFS server cannot
>>> trust the uid/gid coming from QEMU anyway.)
>>>
>>> I think we can merge this patch for QEMU 2.4 but I'd like to have a
>>> discussion about the security risk of encoding libnfs options in the
>>> URI.
>>>
>>> CCed Eric Blake in case libvirt is affected.
>>>
>>> Has anyone thought about this and what are the rules?
>> Good point. In general I think there should be some kind of sanitization of 
>> the parameters
>> before they are passed on to Qemu. In our use case the user cannot pass any 
>> kind of URIs himself,
>> but this might be different in other backends. The readahead value is as 
>> dangerous as well
>> if not sanitized.
>>
>> I had a discussion with Ronnie in the past if we should encode parameters in 
>> the URI or via environment
>> like it is done in libiscsi. If I remember correctly we came up with the URI 
>> parameters for better usability,
>> but hadn't attack scenarios in mind.
>>
>> I am also open to only allow uncritical parameters in the URI and pass 
>> others via a new -nfs cmdline option.
>> Or limit max readahead and max debug level settable via URI.
> I'd feel safer if the option was in runtime_opts instead.  The the
> management tool has to pass them explicitly and the end user cannot
> influence them via the URI.
>
> If an option is needed both at open and create time, then it must also
> be parsed from nfs_file_create() opts.

Ok, I will send a patch that follows this approach. And also a second one to
limit the readahead size to a reasonable value.

Peter