diff mbox

[RFC,v5,08/21] virtagent: add agent_viewfile qmp/hmp command

Message ID 1291399402-20366-9-git-send-email-mdroth@linux.vnet.ibm.com
State New
Headers show

Commit Message

Michael Roth Dec. 3, 2010, 6:03 p.m. UTC
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(-)

Comments

Adam Litke Dec. 6, 2010, 10:08 p.m. UTC | #1
On Fri, 2010-12-03 at 12:03 -0600, 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".

What happens to the monitor if you use this to view a binary file?
Retrieving binary files progmatically using the QMP interface is a valid
use case right?  If so, I don't think it makes sense to introduce
confusion by renaming the rpc function from getfile to viewfile when we
are just exposing the getfile interface.
Michael Roth Dec. 6, 2010, 11:20 p.m. UTC | #2
On 12/06/2010 04:08 PM, Adam Litke wrote:
> On Fri, 2010-12-03 at 12:03 -0600, 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".
>
> What happens to the monitor if you use this to view a binary file?

At the very least we probably get a lot of truncated files from the 
binary->string conversion via monitor_printf(). Im not sure how the 
qobject/json layer would deal with things.

> Retrieving binary files progmatically using the QMP interface is a valid
> use case right?

For getfile (the RPC), but not for viewfile (HMP/QMP). It's doable, but 
we'd *have to* pass this data to the user as base64-encoded data at the 
QMP level. At the HMP level I think we're good either way, since we 
could just base64 decode in the print function.

So in the case of QMP we'd be pushing complexity to the user in exchange 
for not having a seperate plain-text-only interface.

Either way seems reasonable, but I'd been planning on adding a seperate 
`agent_copyfile <remote_path> <local_path>` command for dealing with 
binary data, and making viewfile quick and easy for plain text (both for 
HMP and QMP).
Michael Roth Dec. 7, 2010, 2:09 p.m. UTC | #3
On 12/06/2010 05:20 PM, Michael Roth wrote:
> On 12/06/2010 04:08 PM, Adam Litke wrote:
>> On Fri, 2010-12-03 at 12:03 -0600, 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".
>>
>> What happens to the monitor if you use this to view a binary file?
>
> At the very least we probably get a lot of truncated files from the
> binary->string conversion via monitor_printf(). Im not sure how the
> qobject/json layer would deal with things.
>
>> Retrieving binary files progmatically using the QMP interface is a valid
>> use case right?
>
> For getfile (the RPC), but not for viewfile (HMP/QMP). It's doable, but
> we'd *have to* pass this data to the user as base64-encoded data at the
> QMP level. At the HMP level I think we're good either way, since we
> could just base64 decode in the print function.
>
> So in the case of QMP we'd be pushing complexity to the user in exchange
> for not having a seperate plain-text-only interface.
>
> Either way seems reasonable, but I'd been planning on adding a seperate
> `agent_copyfile <remote_path> <local_path>` command for dealing with
> binary data, and making viewfile quick and easy for plain text (both for
> HMP and QMP).
>

Although, agent_copyfile doesn't seem like the right approach looking at 
things like future libvirt integration. So we will most likely end up 
with a QMP command that passes base64-encoded binary data to the 
end-user for binary data, which we can provide a pretty-printing HMP 
function to decode. We'd need to take care to differentiate the HMP 
command from the QMP one however, else we'd have users tempted to do 
something like:

echo "agent_getfile /remotepath/rand.bin" | socat stdin 
unix-connect:monitor.sock > /localpath/rand.bin

to avoid having to decode the data. Would documenting the HMP 
counterpart as being reliable only for plain-text be sufficient? Or 
Should be have QMP:agent_getfile() and HMP:agent_viewfile()?
Jes Sorensen Dec. 7, 2010, 2:26 p.m. UTC | #4
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.

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

Jes
Michael Roth Dec. 9, 2010, 9:12 p.m. UTC | #5
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
>
Jes Sorensen Dec. 10, 2010, 6:43 a.m. UTC | #6
On 12/09/10 22:12, Michael Roth wrote:
> On 12/07/2010 08:26 AM, Jes Sorensen wrote:
>> 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;
>         }
>     }

You cannot rely on the guest controlling this. You really have to treat
any guest as hostile and keep control and security in the host,
otherwise a hacked guest could end up attacking the host by blowing up
the host's QEMU process.

Cheers,
Jes
Michael Roth Dec. 10, 2010, 5:09 p.m. UTC | #7
On 12/10/2010 12:43 AM, Jes Sorensen wrote:
> On 12/09/10 22:12, Michael Roth wrote:
>> On 12/07/2010 08:26 AM, Jes Sorensen wrote:
>>> 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;
>>          }
>>      }
>
> You cannot rely on the guest controlling this. You really have to treat
> any guest as hostile and keep control and security in the host,
> otherwise a hacked guest could end up attacking the host by blowing up
> the host's QEMU process.

Definetely agree on this, I mentioned some other checks here at the 
transport and host xmlrpc level that would also limit this possibility:

 > 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;
 >              }

I think with strictly enforced size limits the major liability for 
viewfile is, as you mentioned, users using it to view binary data or 
carefully crafted files that can mess up or fool users/shells/programs 
interpreting monitor output.

But plain-text does not include escape sequences, so it's completely 
reasonable that we'd scrape them. And I'm not sure if a "(qemu)" in the 
text is a potential liability. Would there be any other issues to consider?

If we can guard against those things, do you agree it wouldn't be an 
inherently dangerous interface? State-full, asynchronous RPCs like 
copyfile and exec are not really something I'd planned for the initial 
release. I think they'll take some time to get right, and a simple 
low-risk interface to cover what I'm fairly sure is the most common use 
case seems reasonable.

>
> Cheers,
> Jes
Jes Sorensen Dec. 13, 2010, 8:29 a.m. UTC | #8
On 12/10/10 18:09, Michael Roth wrote:
> I think with strictly enforced size limits the major liability for
> viewfile is, as you mentioned, users using it to view binary data or
> carefully crafted files that can mess up or fool users/shells/programs
> interpreting monitor output.
> 
> But plain-text does not include escape sequences, so it's completely
> reasonable that we'd scrape them. And I'm not sure if a "(qemu)" in the
> text is a potential liability. Would there be any other issues to consider?
> 
> If we can guard against those things, do you agree it wouldn't be an
> inherently dangerous interface? State-full, asynchronous RPCs like
> copyfile and exec are not really something I'd planned for the initial
> release. I think they'll take some time to get right, and a simple
> low-risk interface to cover what I'm fairly sure is the most common use
> case seems reasonable.

I am still wary of relying on strict limit enforcement. It is the sort
of thing that will eventually change without us noticing and we end up
with a security hole.

IMHO QEMU should not try to do these sorts of things, instead it should
provide the transport and control services. I don't think file viewing
belongs in QEMU at all. I would be a lot more comfortable if this was
implemented as a standalone monitor interface that connected to QEMU's
QMP interface. I could then use QMP to perform actions like copying the
file to /tmp and if viewing the file caused the monitor to lock up, we
wouldn't lose the guest. This could indeed be the start of an external
monitor :)

Cheers,
Jes
diff mbox

Patch

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;
+    }
+
+    if (file_contents != NULL) {
+        qdict_put(qdict, "contents",
+                  qstring_from_substr(file_contents, 0, file_size-1));
+    }
+
+out:
+    xmlrpc_DECREF(resp);
+out_no_resp:
+    if (mon_cb) {
+        mon_cb(mon_data, QOBJECT(qdict));
+    }
+    qobject_decref(QOBJECT(qdict));
+}
+
+/*
+ * do_agent_viewfile(): View a text file in the guest
+ */
+int do_agent_viewfile(Monitor *mon, const QDict *mon_params,
+                      MonitorCompletion cb, void *opaque)
+{
+    xmlrpc_env env;
+    xmlrpc_value *params;
+    const char *filepath;
+    int ret;
+
+    filepath = qdict_get_str(mon_params, "filepath");
+    xmlrpc_env_init(&env);
+    params = xmlrpc_build_value(&env, "(s)", filepath);
+    if (va_rpc_has_error(&env)) {
+        return -1;
+    }
+
+    ret = va_do_rpc(&env, "va.getfile", params, do_agent_viewfile_cb, cb,
+                    opaque);
+    if (ret) {
+        qerror_report(QERR_VA_FAILED, ret, strerror(ret));
+    }
+    xmlrpc_DECREF(params);
+    return ret;
+}
diff --git a/virtagent.h b/virtagent.h
index af36b6a..5960616 100644
--- a/virtagent.h
+++ b/virtagent.h
@@ -28,5 +28,8 @@  typedef struct VAClientData {
 } VAClientData;
 
 int va_client_init(VAClientData *client_data);
+void do_agent_viewfile_print(Monitor *mon, const QObject *qobject);
+int do_agent_viewfile(Monitor *mon, const QDict *mon_params,
+                      MonitorCompletion cb, void *opaque);
 
 #endif /* VIRTAGENT_H */