[Qemu-devel] Re: [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On 12/07/2010 08:26 AM, Jes Sorensen wrote:
> On 12/03/10 19:03, Michael Roth wrote:
>> Utilize the getfile RPC to provide a means to view text files in the
>> guest. Getfile can handle binary files as well but we don't advertise
>> that here due to the special handling requiring to store it and provide
>> it back to the user (base64 encoding it for instance). Hence the
>> otherwise confusing "viewfile" as opposed to "getfile".
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   hmp-commands.hx |   16 +++++++++
>>   monitor.c       |    1 +
>>   qmp-commands.hx |   33 +++++++++++++++++++
>>   virtagent.c     |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   virtagent.h     |    3 ++
>>   5 files changed, 149 insertions(+), 0 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index e5585ba..423c752 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1212,6 +1212,22 @@ show available trace events and their state
>>   ETEXI
>>   #endif
>>
>> +    {
>> +        .name       = "agent_viewfile",
>> +        .args_type  = "filepath:s",
>> +        .params     = "filepath",
>> +        .help       = "Echo a file from the guest filesystem",
>> +        .user_print = do_agent_viewfile_print,
>> +        .mhandler.cmd_async = do_agent_viewfile,
>> +        .flags      = MONITOR_CMD_ASYNC,
>> +    },
>> +
>> +STEXI
>> +@item agent_viewfile @var{filepath}
>> +@findex agent_viewfile
>> +Echo the file identified by @var{filepath} on the guest filesystem
>> +ETEXI
>> +
>>   STEXI
>>   @end table
>>   ETEXI
>> diff --git a/monitor.c b/monitor.c
>> index 8cee35d..145895d 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -56,6 +56,7 @@
>>   #include "json-parser.h"
>>   #include "osdep.h"
>>   #include "exec-all.h"
>> +#include "virtagent.h"
>>   #ifdef CONFIG_SIMPLE_TRACE
>>   #include "trace.h"
>>   #endif
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 793cf1c..efa2137 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -738,6 +738,39 @@ Example:
>>   EQMP
>>
>>       {
>> +        .name       = "agent_viewfile",
>> +        .args_type  = "filepath:s",
>> +        .params     = "filepath",
>> +        .help       = "Echo a file from the guest filesystem",
>> +        .user_print = monitor_user_noop,
>> +        .mhandler.cmd_async = do_agent_viewfile,
>> +        .flags      = MONITOR_CMD_ASYNC,
>> +    },
>> +
>> +STEXI
>> +@item agent_viewfile @var{filepath}
>> +@findex agent_viewfile
>> +Echo the file identified by @var{filepath} on the guest filesystem
>> +ETEXI
>> +SQMP
>> +agent_viewfile
>> +--------
>> +
>> +Echo the file identified by @var{filepath} from the guest filesystem.
>> +
>> +Arguments:
>> +
>> +- "filepath": Full guest path of the desired file
>> +
>> +Example:
>> +
>> +->  { "execute": "agent_viewfile",
>> +                "arguments": { "filepath": "/sys/kernel/kexec_loaded" } }
>> +<- { "return": { "contents": "0" } }
>> +
>> +EQMP
>> +
>> +    {
>>           .name       = "qmp_capabilities",
>>           .args_type  = "",
>>           .params     = "",
>> diff --git a/virtagent.c b/virtagent.c
>> index 34d8545..4a4dc8a 100644
>> --- a/virtagent.c
>> +++ b/virtagent.c
>> @@ -139,3 +139,99 @@ out_free:
>>   out:
>>       return ret;
>>   }
>> +
>> +/* QMP/HMP RPC client functions */
>> +
>> +void do_agent_viewfile_print(Monitor *mon, const QObject *data)
>> +{
>> +    QDict *qdict;
>> +    const char *contents = NULL;
>> +    int i;
>> +
>> +    qdict = qobject_to_qdict(data);
>> +    if (!qdict_haskey(qdict, "contents")) {
>> +        return;
>> +    }
>> +
>> +    contents = qdict_get_str(qdict, "contents");
>> +    if (contents != NULL) {
>> +         /* monitor_printf truncates so do it in chunks. also, file_contents
>> +          * may not be null-termed at proper location so explicitly calc
>> +          * last chunk sizes */
>> +        for (i = 0; i<  strlen(contents); i += 1024) {
>> +            monitor_printf(mon, "%.1024s", contents + i);
>> +        }
>> +    }
>> +    monitor_printf(mon, "\n");
>> +}
>> +
>> +static void do_agent_viewfile_cb(const char *resp_data,
>> +                                 size_t resp_data_len,
>> +                                 MonitorCompletion *mon_cb,
>> +                                 void *mon_data)
>> +{
>> +    xmlrpc_value *resp = NULL;
>> +    char *file_contents = NULL;
>> +    size_t file_size;
>> +    int ret;
>> +    xmlrpc_env env;
>> +    QDict *qdict = qdict_new();
>> +
>> +    if (resp_data == NULL) {
>> +        LOG("error handling RPC request");
>> +        goto out_no_resp;
>> +    }
>> +
>> +    xmlrpc_env_init(&env);
>> +    resp = xmlrpc_parse_response(&env, resp_data, resp_data_len);
>> +    if (va_rpc_has_error(&env)) {
>> +        ret = -1;
>> +        goto out_no_resp;
>> +    }
>> +
>> +    xmlrpc_parse_value(&env, resp, "6",&file_contents,&file_size);
>> +    if (va_rpc_has_error(&env)) {
>> +        ret = -1;
>> +        goto out;
>
> I believe this suffers from the same architectural problem I mentioned
> in my comment to 07/21 - you don't restrict the file size, so it could
> blow up the QEMU process on the host trying to view the wrong file.

It's restricted on the guest side:

virtagent-server.c:va_getfile():

     while ((ret = read(fd, buf, VA_FILEBUF_LEN)) > 0) {
         file_contents = qemu_realloc(file_contents, count + 
VA_FILEBUF_LEN);
         memcpy(file_contents + count, buf, ret);
         count += ret;
         if (count > VA_GETFILE_MAX) {
             xmlrpc_faultf(env, "max file size (%d bytes) exceeded",
                           VA_GETFILE_MAX);
             goto EXIT_CLOSE_BAD;
         }
     }

There are additional limits at the transport layer well to deal with a 
potentially malicious/buggy agent as well:

virtagent-common.c:va_http_read_handler():

             } else if (s->content_len > VA_CONTENT_LEN_MAX) {
                 LOG("http content length too long");
                 goto out_bad;
             }

And configurable limits enforced by the xmlrpc-c library on the host and 
guest side, which I haven't been explicitly setting or keeping 
consistent with the other various limits. I'll address this for the next 
round.

>
> I really think it is a bad idea to put this kind of command into the
> monitor.
>
> Jes
>