[libvirt] [PATCH] virDomainMemoryPeek - peek into guest memory

[Daniel P. Berrange]

On Thu, Jun 05, 2008 at 10:30:55PM +0100, Richard W.M. Jones wrote:
>  
> +/* Memory peeking flags. */
> +typedef enum {
> +  VIR_MEMORY_VIRTUAL              = 1, /* addresses are virtual addresses */
> +} virDomainMemoryFlags;

Since there is only one flag, and it is compulsory I'm rather inclined
to say that virtual memory addressing should be the default with a flags
value of 0. Unless there is another mode, not yet implemented, that you 
think would be a better default in the future ?  Obviously keep the flags
arg for expansion regardless though.

> diff -u -p -r1.36 remote.c
> --- qemud/remote.c	5 Jun 2008 21:12:26 -0000	1.36
> +++ qemud/remote.c	5 Jun 2008 21:26:29 -0000
> @@ -938,6 +938,52 @@ remoteDispatchDomainBlockPeek (struct qe
>  }
>  
>  static int
> +remoteDispatchDomainMemoryPeek (struct qemud_server *server ATTRIBUTE_UNUSED,
> +                                struct qemud_client *client,
> +                                remote_message_header *req,
> +                                remote_domain_memory_peek_args *args,
> +                                remote_domain_memory_peek_ret *ret)
> +{
> +    virDomainPtr dom;
> +    unsigned long long offset;
> +    size_t size;
> +    unsigned int flags;
> +    CHECK_CONN (client);
> +
> +    dom = get_nonnull_domain (client->conn, args->dom);
> +    if (dom == NULL) {
> +        remoteDispatchError (client, req, "%s", _("domain not found"));
> +        return -2;
> +    }
> +    offset = args->offset;
> +    size = args->size;
> +    flags = args->flags;
> +
> +    if (size > REMOTE_DOMAIN_BLOCK_PEEK_BUFFER_MAX) {
> +        remoteDispatchError (client, req,
> +                             "%s", _("size > maximum buffer size"));
> +        return -2;
> +    }
> +
> +    ret->buffer.buffer_len = size;
> +    ret->buffer.buffer_val = malloc (size);
> +    if (!ret->buffer.buffer_val) {
> +        remoteDispatchError (client, req, "%s", strerror (errno));
> +        return -2;
> +    }

The 'dom' object is leaking in the 2 error paths here & it'd be better
to use VIR_ALLOC_N(ret->buffer.buffer_val, size) here.

> diff -u -p -r1.14 remote_protocol.x
> --- qemud/remote_protocol.x	5 Jun 2008 21:12:27 -0000	1.14
> +++ qemud/remote_protocol.x	5 Jun 2008 21:26:29 -0000
> @@ -340,6 +340,17 @@ struct remote_domain_block_peek_ret {
>      opaque buffer<REMOTE_DOMAIN_BLOCK_PEEK_BUFFER_MAX>;
>  };
>  
> +struct remote_domain_memory_peek_args {
> +    remote_nonnull_domain dom;
> +    unsigned hyper offset;
> +    unsigned size;
> +    unsigned flags;
> +};
> +
> +struct remote_domain_memory_peek_ret {
> +    opaque buffer<REMOTE_DOMAIN_BLOCK_PEEK_BUFFER_MAX>;
> +};

Is it worth having a separate constant for the max memory size, independant
of the block peek size. 

> +/**
> + * virDomainMemoryPeek:
> + * @dom: pointer to the domain object
> + * @start: start of memory to peek
> + * @size: size of memory to peek
> + * @buffer: return buffer (must be at least size bytes)
> + * @flags: flags, see below
> + *
> + * This function allows you to read the contents of a domain's
> + * memory.
> + *
> + * The memory which is read is controlled by the 'start', 'size'
> + * and 'flags' parameters.
> + *
> + * If 'flags' is VIR_MEMORY_VIRTUAL then the 'start' and 'size'
> + * parameters are interpreted as virtual memory addresses for
> + * whichever task happens to be running on the domain at the
> + * moment.  Although this sounds haphazard it is in fact what
> + * you want in order to read Linux kernel state, because it
> + * ensures that pointers in the kernel image can be interpreted
> + * coherently.
> + *
> + * 'buffer' is the return buffer and must be at least 'size' bytes.
> + * 'size' may be 0 to test if the call would succeed.

Should we document and enforce a maximum value for size. The remote
driver at least has a maximum limit,, so perhaps we should enforce
that in the main API dispatcher here in virDomainMemoryPeek() impl

> +int
> +static int
> +qemudDomainMemoryPeek (virDomainPtr dom,
> +                       unsigned long long offset, size_t size,
> +                       void *buffer,
> +                       unsigned int flags)
> +{
> +    struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
> +    struct qemud_vm *vm = qemudFindVMByID (driver, dom->id);
> +    char cmd[256], *info;
> +    char tmp[] = "/tmp/qemumemXXXXXX";

It'd make the SELinux policy easier if we avoided the generic /tmp and put
the files in somewhere like /var/spool/libvirt/qemu/. Wouldn't even need to
use mkstemp() then - just have it named VMNAME.mem since it'd be in a safe
controlled directory

Regards,
Daniel.
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|