Re: [Qemu-devel] [PATCH V2 03/12] hw/9pfs: File system helper process for qemu 9p proxy FS

[Stefan Hajnoczi]

On Tue, Nov 15, 2011 at 11:57 AM, M. Mohan Kumar <address@hidden> wrote:
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> new file mode 100644
> index 0000000..69daf7c
> --- /dev/null
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -0,0 +1,271 @@
> +/*
> + * Helper for QEMU Proxy FS Driver
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + * M. Mohan Kumar <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + */
> +#include <stdio.h>
> +#include <sys/socket.h>
> +#include <string.h>
> +#include <sys/un.h>
> +#include <limits.h>
> +#include <signal.h>
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <sys/resource.h>
> +#include <sys/stat.h>
> +#include <getopt.h>
> +#include <unistd.h>
> +#include <syslog.h>
> +#include <sys/prctl.h>
> +#include <sys/capability.h>
> +#include <sys/fsuid.h>
> +#include <stdarg.h>
> +#include "bswap.h"

Where is "bswap.h" used and why above <sys/socket.h>?

> +#include <sys/socket.h>
> +#include "qemu-common.h"
> +#include "virtio-9p-marshal.h"
> +#include "hw/9pfs/virtio-9p-proxy.h"
> +
> +#define PROGNAME "virtfs-proxy-helper"
> +
> +static struct option helper_opts[] = {
> +    {"fd", required_argument, NULL, 'f'},
> +    {"path", required_argument, NULL, 'p'},
> +    {"nodaemon", no_argument, NULL, 'n'},
> +};
> +
> +int is_daemon;

static?

Also, please use the bool type from <stdbool.h>, it makes it easier
for readers who don't have to guess how the variable works (might be a
bitfield or reference count too).

> +static int socket_read(int sockfd, void *buff, ssize_t size)
> +{
> +    int retval;
> +
> +    do {
> +        retval = read(sockfd, buff, size);
> +    } while (retval < 0 && errno == EINTR);
> +    if (retval != size) {
> +        return -EIO;
> +    }

Shouldn't this loop until size bytes have been read?

> +    return retval;
> +}
> +
> +static int socket_write(int sockfd, void *buff, ssize_t size)
> +{
> +    int retval;
> +
> +    do {
> +        retval = write(sockfd, buff, size);
> +    } while (retval < 0 && errno == EINTR);
> +    if (retval != size) {
> +        return -EIO;

We could pass the actual -errno here if retval < 0.

> +    }
> +    return retval;
> +}
> +
> +static int read_request(int sockfd, struct iovec *iovec)
> +{
> +    int retval;
> +    ProxyHeader header;
> +
> +    /* read the header */
> +    retval = socket_read(sockfd, iovec->iov_base, sizeof(header));
> +    if (retval != sizeof(header)) {
> +        return -EIO;
> +    }
> +    /* unmarshal header */
> +    proxy_unmarshal(iovec, 1, 0, "dd", &header.type, &header.size);
> +    /* read the request */
> +    retval = socket_read(sockfd, iovec->iov_base + sizeof(header), 
> header.size);
> +    if (retval != header.size) {
> +        return -EIO;
> +    }
> +    return header.type;
> +}

Size checks are missing and we're trusting what the client sends!

> +
> +static void usage(char *prog)
> +{
> +    fprintf(stderr, "usage: %s\n"
> +            " -p|--path <path> 9p path to export\n"
> +            " {-f|--fd <socket-descriptor>} socket file descriptor to be 
> used\n"
> +            " [-n|--nodaemon] Run as a normal program\n",
> +            basename(prog));
> +}
> +
> +static int process_requests(int sock)
> +{
> +    int type;
> +    struct iovec iovec;
> +
> +    iovec.iov_base = g_malloc(BUFF_SZ);
> +    iovec.iov_len = BUFF_SZ;
> +    while (1) {
> +        type = read_request(sock, &iovec);
> +        if (type <= 0) {
> +            goto error;
> +        }
> +    }
> +    (void)socket_write;
> +error:
> +    g_free(iovec.iov_base);
> +    return -1;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    int sock;
> +    char rpath[PATH_MAX];
> +    struct stat stbuf;
> +    int c, option_index;
> +
> +    is_daemon = 1;
> +    rpath[0] = '\0';
> +    sock = -1;
> +    while (1) {
> +        option_index = 0;
> +        c = getopt_long(argc, argv, "p:nh?f:", helper_opts,
> +                        &option_index);
> +        if (c == -1) {
> +            break;
> +        }
> +        switch (c) {
> +        case 'p':
> +            strcpy(rpath, optarg);

Buffer overflow.  The whole thing would be simpler like this:

const char *rpath = "";
[...]
    case 'p':
        rpath = optarg;
        break;

> +            break;
> +        case 'n':
> +            is_daemon = 0;
> +            break;
> +        case 'f':
> +            sock = atoi(optarg);
> +            break;
> +        case '?':
> +        case 'h':
> +        default:
> +            usage(argv[0]);
> +            return -1;

The convention is for programs to exit with 1 (EXIT_FAILURE) on error.

> +            break;
> +        }
> +    }
> +
> +    /* Parameter validation */
> +    if (sock == -1 || rpath[0] == '\0') {
> +        fprintf(stderr, "socket descriptor or path not specified\n");
> +        usage(argv[0]);
> +        return -1;
> +    }
> +
> +    if (lstat(rpath, &stbuf) < 0) {
> +        fprintf(stderr, "invalid path \"%s\" specified?\n", rpath);

sterror() would provide further details on what went wrong.

> +        return -1;
> +    }
> +
> +    if (!S_ISDIR(stbuf.st_mode)) {
> +        fprintf(stderr, "specified path \"%s\" is not directory\n", rpath);
> +        return -1;
> +    }
> +
> +    if (is_daemon) {
> +        if (daemon(0, 0) < 0) {
> +            fprintf(stderr, "daemon call failed\n");
> +            return -1;
> +        }
> +        openlog(PROGNAME, LOG_PID, LOG_DAEMON);
> +    }
> +
> +    do_log(LOG_INFO, "Started");
> +
> +    if (chroot(rpath) < 0) {
> +        do_perror("chroot");
> +        goto error;
> +    }
> +    umask(0);
> +
> +    if (init_capabilities() < 0) {
> +        goto error;
> +    }
> +
> +    process_requests(sock);
> +error:
> +    do_log(LOG_INFO, "Done");
> +    closelog();
> +    return 0;
> +}
> diff --git a/hw/9pfs/virtio-9p-proxy.h b/hw/9pfs/virtio-9p-proxy.h
> index f5e1a02..120e940 100644
> --- a/hw/9pfs/virtio-9p-proxy.h
> +++ b/hw/9pfs/virtio-9p-proxy.h
> @@ -3,6 +3,16 @@
>
>  #define BUFF_SZ (4 * 1024)
>
> +#define proxy_unmarshal(in_sg, in_elem, offset, fmt, args...) \
> +    v9fs_unmarshal(in_sg, in_elem, offset, 0 /* convert */, fmt, ##args)
> +#define proxy_marshal(out_sg, out_elem, offset, fmt, args...) \
> +    v9fs_marshal(out_sg, out_elem, offset, 0 /* convert */, fmt, ##args)
> +
> +union MsgControl {
> +    struct cmsghdr cmsg;
> +    char control[CMSG_SPACE(sizeof(int))];
> +};

This union isn't used in this patch.

Stefan