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

[Stefan Hajnoczi]

On Wed, Nov 16, 2011 at 8:51 AM, M. Mohan Kumar <address@hidden> wrote:
> Stefan Hajnoczi wrote:
>>> +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.
>>
>>
>
> Socket errors are treated fatal and the reason for failures are not used
> by the code. When ever there is socket error, helper exits.

Exiting may be fine but errno or strerror(errno) needs to be printed
on exit.  Otherwise we need to strace it to figure out what happened -
and the error exit may be hard to reproduce, so this is not
attractive.  Therefore it makes sense to return the full -errno and
print it out when exiting.

>>> +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!
>>
>
> Could you please elaborate?

This function allows the client to overflow the iovec buffer.  Check
what happens when header.size > 4096.

I'm alarmed by this because there are 9 more patches to review and
this is just the foundation of the helper.  If the helper isn't secure
then isolating it from QEMU is useless since a malicious QEMU can take
over the helper.  Input needs to be validated.

Stefan