| Message ID | 148976155089.11859.11860739290129101204.stgit@bahia |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 17, 2017 at 03:39:10PM +0100, Greg Kurz wrote: > Replies from the virtfs proxy are made up of a fixed-size header (8 bytes) > and a payload of variable size (maximum 64kb). When receiving a reply, > the proxy backend first reads the whole header and then unmarshals it. > If the header is okay, it then does the same operation with the payload. > > Since the proxy backend uses a pre-allocated buffer which has enough room > for a header and the maximum payload size, marshalling should never fail > with fixed size arguments. Any error here is likely to result from a more > serious corruption in QEMU and we'd better dump core right away. > > This patch adds error checks where they are missing and converts the > associated error paths into assertions. > > Note that we don't want to use sizeof() in the checks since the value > we want to use depends on the format rather than the size of the buffer. > Short marshal formats can be handled with numerical values. Size macros > are introduced for bigger ones (ProxyStat and ProxyStatFS). > > This should also address Coverity's complaints CID 1348519 and CID 1348520, > about not always checking the return value of proxy_unmarshal(). > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > v2: - added PROXY_STAT_SZ and PROXY_STATFS_SZ macros > - updated changelog > --- > hw/9pfs/9p-proxy.c | 24 +++++++++++++----------- > hw/9pfs/9p-proxy.h | 10 ++++++++-- > 2 files changed, 21 insertions(+), 13 deletions(-) > > diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c > index f4aa7a9d70f8..363bea66035e 100644 > --- a/hw/9pfs/9p-proxy.c > +++ b/hw/9pfs/9p-proxy.c > @@ -165,7 +165,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type, > return retval; > } > reply->iov_len = PROXY_HDR_SZ; > - proxy_unmarshal(reply, 0, "dd", &header.type, &header.size); > + retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size); > + assert(retval == 8); > /* > * if response size > PROXY_MAX_IO_SZ, read the response but ignore it and > * return -ENOBUFS > @@ -194,15 +195,14 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type, > if (header.type == T_ERROR) { > int ret; > ret = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status); > - if (ret < 0) { > - *status = ret; > - } > + assert(ret == 4); > return 0; > } > > switch (type) { > case T_LSTAT: { > ProxyStat prstat; > + QEMU_BUILD_BUG_ON(sizeof(prstat) != PROXY_STAT_SZ); I'd just put this assert at the struct declaration ..snip... > diff --git a/hw/9pfs/9p-proxy.h b/hw/9pfs/9p-proxy.h > index b84301d001b0..918c45016a3c 100644 > --- a/hw/9pfs/9p-proxy.h > +++ b/hw/9pfs/9p-proxy.h > @@ -79,7 +79,10 @@ typedef struct { > uint64_t st_mtim_nsec; > uint64_t st_ctim_sec; > uint64_t st_ctim_nsec; > -} ProxyStat; > +} QEMU_PACKED ProxyStat; > + > +#define PROXY_STAT_SZ \ > + (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8) eg. QEMU_BUILD_BUG_ON(sizeof(ProxyStat) != (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8)); Regards, Daniel
On 03/17/2017 11:43 AM, Daniel P. Berrange wrote: > On Fri, Mar 17, 2017 at 03:39:10PM +0100, Greg Kurz wrote: >> Replies from the virtfs proxy are made up of a fixed-size header (8 bytes) >> and a payload of variable size (maximum 64kb). When receiving a reply, >> the proxy backend first reads the whole header and then unmarshals it. >> If the header is okay, it then does the same operation with the payload. >> >> Since the proxy backend uses a pre-allocated buffer which has enough room >> for a header and the maximum payload size, marshalling should never fail >> with fixed size arguments. Any error here is likely to result from a more >> serious corruption in QEMU and we'd better dump core right away. >> >> This patch adds error checks where they are missing and converts the >> associated error paths into assertions. >> >> Note that we don't want to use sizeof() in the checks since the value >> we want to use depends on the format rather than the size of the buffer. >> Short marshal formats can be handled with numerical values. Size macros >> are introduced for bigger ones (ProxyStat and ProxyStatFS). :) maybe this comment should go somewhere in <hw/9pfs/9p-proxy.h> >> This should also address Coverity's complaints CID 1348519 and CID 1348520, >> about not always checking the return value of proxy_unmarshal(). >> >> Signed-off-by: Greg Kurz <groug@kaod.org> >> --- >> v2: - added PROXY_STAT_SZ and PROXY_STATFS_SZ macros >> - updated changelog >> --- >> hw/9pfs/9p-proxy.c | 24 +++++++++++++----------- >> hw/9pfs/9p-proxy.h | 10 ++++++++-- >> 2 files changed, 21 insertions(+), 13 deletions(-) >> >> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c >> index f4aa7a9d70f8..363bea66035e 100644 >> --- a/hw/9pfs/9p-proxy.c >> +++ b/hw/9pfs/9p-proxy.c >> @@ -165,7 +165,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type, >> return retval; >> } >> reply->iov_len = PROXY_HDR_SZ; >> - proxy_unmarshal(reply, 0, "dd", &header.type, &header.size); >> + retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size); >> + assert(retval == 8); >> /* >> * if response size > PROXY_MAX_IO_SZ, read the response but ignore it and >> * return -ENOBUFS >> @@ -194,15 +195,14 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type, >> if (header.type == T_ERROR) { >> int ret; >> ret = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status); >> - if (ret < 0) { >> - *status = ret; >> - } >> + assert(ret == 4); >> return 0; >> } >> >> switch (type) { >> case T_LSTAT: { >> ProxyStat prstat; >> + QEMU_BUILD_BUG_ON(sizeof(prstat) != PROXY_STAT_SZ); > > I'd just put this assert at the struct declaration > > ..snip... > >> diff --git a/hw/9pfs/9p-proxy.h b/hw/9pfs/9p-proxy.h >> index b84301d001b0..918c45016a3c 100644 >> --- a/hw/9pfs/9p-proxy.h >> +++ b/hw/9pfs/9p-proxy.h >> @@ -79,7 +79,10 @@ typedef struct { >> uint64_t st_mtim_nsec; >> uint64_t st_ctim_sec; >> uint64_t st_ctim_nsec; >> -} ProxyStat; >> +} QEMU_PACKED ProxyStat; >> + >> +#define PROXY_STAT_SZ \ >> + (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8) > > eg. > > QEMU_BUILD_BUG_ON(sizeof(ProxyStat) != > (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8)); or QEMU_BUILD_BUG_ON(sizeof(ProxyStat) != proxy_unmarshal_calcsize("qqqdddqqqqqqqqqq")); or eventually stricter using some gcc/clang Statement Expressions: #define proxy_unmarshal_strict(in_sg, in_sg_sz, offset, fmt, args...) \ ({ \ ssize_t retval; \ retval = v9fs_iov_unmarshal(in_sg, 1, offset, 0, fmt, ##args); \ QEMU_BUILD_BUG_ON(in_sg_sz != proxy_unmarshal_calcsize(fmt)); \ retval; \ }) so we could use: retval = proxy_unmarshal_strict(reply, sizeof(reply), 0, "dd", &header.type, &header.size); > > > Regards, > Daniel >
On Fri, 17 Mar 2017 14:43:00 +0000 "Daniel P. Berrange" <berrange@redhat.com> wrote: > On Fri, Mar 17, 2017 at 03:39:10PM +0100, Greg Kurz wrote: > > Replies from the virtfs proxy are made up of a fixed-size header (8 bytes) > > and a payload of variable size (maximum 64kb). When receiving a reply, > > the proxy backend first reads the whole header and then unmarshals it. > > If the header is okay, it then does the same operation with the payload. > > > > Since the proxy backend uses a pre-allocated buffer which has enough room > > for a header and the maximum payload size, marshalling should never fail > > with fixed size arguments. Any error here is likely to result from a more > > serious corruption in QEMU and we'd better dump core right away. > > > > This patch adds error checks where they are missing and converts the > > associated error paths into assertions. > > > > Note that we don't want to use sizeof() in the checks since the value > > we want to use depends on the format rather than the size of the buffer. > > Short marshal formats can be handled with numerical values. Size macros > > are introduced for bigger ones (ProxyStat and ProxyStatFS). > > > > This should also address Coverity's complaints CID 1348519 and CID 1348520, > > about not always checking the return value of proxy_unmarshal(). > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > v2: - added PROXY_STAT_SZ and PROXY_STATFS_SZ macros > > - updated changelog > > --- > > hw/9pfs/9p-proxy.c | 24 +++++++++++++----------- > > hw/9pfs/9p-proxy.h | 10 ++++++++-- > > 2 files changed, 21 insertions(+), 13 deletions(-) > > > > diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c > > index f4aa7a9d70f8..363bea66035e 100644 > > --- a/hw/9pfs/9p-proxy.c > > +++ b/hw/9pfs/9p-proxy.c > > @@ -165,7 +165,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type, > > return retval; > > } > > reply->iov_len = PROXY_HDR_SZ; > > - proxy_unmarshal(reply, 0, "dd", &header.type, &header.size); > > + retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size); > > + assert(retval == 8); > > /* > > * if response size > PROXY_MAX_IO_SZ, read the response but ignore it and > > * return -ENOBUFS > > @@ -194,15 +195,14 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type, > > if (header.type == T_ERROR) { > > int ret; > > ret = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status); > > - if (ret < 0) { > > - *status = ret; > > - } > > + assert(ret == 4); > > return 0; > > } > > > > switch (type) { > > case T_LSTAT: { > > ProxyStat prstat; > > + QEMU_BUILD_BUG_ON(sizeof(prstat) != PROXY_STAT_SZ); > > I'd just put this assert at the struct declaration > > ..snip... > > > diff --git a/hw/9pfs/9p-proxy.h b/hw/9pfs/9p-proxy.h > > index b84301d001b0..918c45016a3c 100644 > > --- a/hw/9pfs/9p-proxy.h > > +++ b/hw/9pfs/9p-proxy.h > > @@ -79,7 +79,10 @@ typedef struct { > > uint64_t st_mtim_nsec; > > uint64_t st_ctim_sec; > > uint64_t st_ctim_nsec; > > -} ProxyStat; > > +} QEMU_PACKED ProxyStat; > > + > > +#define PROXY_STAT_SZ \ > > + (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8) > > eg. > > QEMU_BUILD_BUG_ON(sizeof(ProxyStat) != > (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8)); > Makes sense. I'll send a v3. Thanks. -- Greg > > Regards, > Daniel
On Fri, 17 Mar 2017 12:06:39 -0300 Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > On 03/17/2017 11:43 AM, Daniel P. Berrange wrote: > > On Fri, Mar 17, 2017 at 03:39:10PM +0100, Greg Kurz wrote: > >> Replies from the virtfs proxy are made up of a fixed-size header (8 bytes) > >> and a payload of variable size (maximum 64kb). When receiving a reply, > >> the proxy backend first reads the whole header and then unmarshals it. > >> If the header is okay, it then does the same operation with the payload. > >> > >> Since the proxy backend uses a pre-allocated buffer which has enough room > >> for a header and the maximum payload size, marshalling should never fail > >> with fixed size arguments. Any error here is likely to result from a more > >> serious corruption in QEMU and we'd better dump core right away. > >> > >> This patch adds error checks where they are missing and converts the > >> associated error paths into assertions. > >> > >> Note that we don't want to use sizeof() in the checks since the value > >> we want to use depends on the format rather than the size of the buffer. > >> Short marshal formats can be handled with numerical values. Size macros > >> are introduced for bigger ones (ProxyStat and ProxyStatFS). > > :) maybe this comment should go somewhere in <hw/9pfs/9p-proxy.h> > > >> This should also address Coverity's complaints CID 1348519 and CID 1348520, > >> about not always checking the return value of proxy_unmarshal(). > >> > >> Signed-off-by: Greg Kurz <groug@kaod.org> > >> --- > >> v2: - added PROXY_STAT_SZ and PROXY_STATFS_SZ macros > >> - updated changelog > >> --- > >> hw/9pfs/9p-proxy.c | 24 +++++++++++++----------- > >> hw/9pfs/9p-proxy.h | 10 ++++++++-- > >> 2 files changed, 21 insertions(+), 13 deletions(-) > >> > >> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c > >> index f4aa7a9d70f8..363bea66035e 100644 > >> --- a/hw/9pfs/9p-proxy.c > >> +++ b/hw/9pfs/9p-proxy.c > >> @@ -165,7 +165,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type, > >> return retval; > >> } > >> reply->iov_len = PROXY_HDR_SZ; > >> - proxy_unmarshal(reply, 0, "dd", &header.type, &header.size); > >> + retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size); > >> + assert(retval == 8); > >> /* > >> * if response size > PROXY_MAX_IO_SZ, read the response but ignore it and > >> * return -ENOBUFS > >> @@ -194,15 +195,14 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type, > >> if (header.type == T_ERROR) { > >> int ret; > >> ret = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status); > >> - if (ret < 0) { > >> - *status = ret; > >> - } > >> + assert(ret == 4); > >> return 0; > >> } > >> > >> switch (type) { > >> case T_LSTAT: { > >> ProxyStat prstat; > >> + QEMU_BUILD_BUG_ON(sizeof(prstat) != PROXY_STAT_SZ); > > > > I'd just put this assert at the struct declaration > > > > ..snip... > > > >> diff --git a/hw/9pfs/9p-proxy.h b/hw/9pfs/9p-proxy.h > >> index b84301d001b0..918c45016a3c 100644 > >> --- a/hw/9pfs/9p-proxy.h > >> +++ b/hw/9pfs/9p-proxy.h > >> @@ -79,7 +79,10 @@ typedef struct { > >> uint64_t st_mtim_nsec; > >> uint64_t st_ctim_sec; > >> uint64_t st_ctim_nsec; > >> -} ProxyStat; > >> +} QEMU_PACKED ProxyStat; > >> + > >> +#define PROXY_STAT_SZ \ > >> + (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8) > > > > eg. > > > > QEMU_BUILD_BUG_ON(sizeof(ProxyStat) != > > (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8)); > > or > > QEMU_BUILD_BUG_ON(sizeof(ProxyStat) != > proxy_unmarshal_calcsize("qqqdddqqqqqqqqqq")); > The purpose of QEMU_BUILD_BUG_ON(x) is to break the build if the condition is false. It expands to something like: typedef struct { int:(cond) ? -1 : 1; } qemu_build_bug_on__5 __attribute__((unused)); and causes gcc to fail if cond is 0: error: negative width in bit-field ‘<anonymous>’ This really only makes sense if cond is constant, otherwise gcc complains with: error: bit-field ‘<anonymous>’ width not an integer constant And I can't think of any way of implementing proxy_unmarshal_calcsize() so that it boils down to a constant at build time. Also, the protocol is stable and won't ever change: it is ok to compute the sizes once and for all. > or eventually stricter using some gcc/clang Statement Expressions: > > #define proxy_unmarshal_strict(in_sg, in_sg_sz, offset, fmt, args...) \ > ({ \ > ssize_t retval; \ > retval = v9fs_iov_unmarshal(in_sg, 1, offset, 0, fmt, ##args); \ > QEMU_BUILD_BUG_ON(in_sg_sz != proxy_unmarshal_calcsize(fmt)); \ This should be an assert() actually, and, again, proxy_unmarshal_calcsize() cannot be a constant, and I certainly don't want to do anything but comparing two integers here at runtime. > retval; \ > }) > > so we could use: > > retval = proxy_unmarshal_strict(reply, sizeof(reply), 0, "dd", > &header.type, &header.size); > I wanted to do something like this initially but I decided not to because of the reasons exposed above. But if you have a solution, I'll gladly give a try. :) Thanks -- Greg > > > > > > Regards, > > Daniel > >
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c index f4aa7a9d70f8..363bea66035e 100644 --- a/hw/9pfs/9p-proxy.c +++ b/hw/9pfs/9p-proxy.c @@ -165,7 +165,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type, return retval; } reply->iov_len = PROXY_HDR_SZ; - proxy_unmarshal(reply, 0, "dd", &header.type, &header.size); + retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size); + assert(retval == 8); /* * if response size > PROXY_MAX_IO_SZ, read the response but ignore it and * return -ENOBUFS @@ -194,15 +195,14 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type, if (header.type == T_ERROR) { int ret; ret = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status); - if (ret < 0) { - *status = ret; - } + assert(ret == 4); return 0; } switch (type) { case T_LSTAT: { ProxyStat prstat; + QEMU_BUILD_BUG_ON(sizeof(prstat) != PROXY_STAT_SZ); retval = proxy_unmarshal(reply, PROXY_HDR_SZ, "qqqdddqqqqqqqqqq", &prstat.st_dev, &prstat.st_ino, &prstat.st_nlink, @@ -213,11 +213,13 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type, &prstat.st_atim_sec, &prstat.st_atim_nsec, &prstat.st_mtim_sec, &prstat.st_mtim_nsec, &prstat.st_ctim_sec, &prstat.st_ctim_nsec); + assert(retval == PROXY_STAT_SZ); prstat_to_stat(response, &prstat); break; } case T_STATFS: { ProxyStatFS prstfs; + QEMU_BUILD_BUG_ON(sizeof(prstfs) != PROXY_STATFS_SZ); retval = proxy_unmarshal(reply, PROXY_HDR_SZ, "qqqqqqqqqqq", &prstfs.f_type, &prstfs.f_bsize, &prstfs.f_blocks, @@ -225,6 +227,7 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type, &prstfs.f_files, &prstfs.f_ffree, &prstfs.f_fsid[0], &prstfs.f_fsid[1], &prstfs.f_namelen, &prstfs.f_frsize); + assert(retval == PROXY_STATFS_SZ); prstatfs_to_statfs(response, &prstfs); break; } @@ -246,7 +249,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type, break; } case T_GETVERSION: - proxy_unmarshal(reply, PROXY_HDR_SZ, "q", response); + retval = proxy_unmarshal(reply, PROXY_HDR_SZ, "q", response); + assert(retval == 8); break; default: return -1; @@ -274,18 +278,16 @@ static int v9fs_receive_status(V9fsProxy *proxy, return retval; } reply->iov_len = PROXY_HDR_SZ; - proxy_unmarshal(reply, 0, "dd", &header.type, &header.size); - if (header.size != sizeof(int)) { - *status = -ENOBUFS; - return 0; - } + retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size); + assert(retval == 8); retval = socket_read(proxy->sockfd, reply->iov_base + PROXY_HDR_SZ, header.size); if (retval < 0) { return retval; } reply->iov_len += header.size; - proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status); + retval = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status); + assert(retval == 4); return 0; } diff --git a/hw/9pfs/9p-proxy.h b/hw/9pfs/9p-proxy.h index b84301d001b0..918c45016a3c 100644 --- a/hw/9pfs/9p-proxy.h +++ b/hw/9pfs/9p-proxy.h @@ -79,7 +79,10 @@ typedef struct { uint64_t st_mtim_nsec; uint64_t st_ctim_sec; uint64_t st_ctim_nsec; -} ProxyStat; +} QEMU_PACKED ProxyStat; + +#define PROXY_STAT_SZ \ + (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8) typedef struct { uint64_t f_type; @@ -92,5 +95,8 @@ typedef struct { uint64_t f_fsid[2]; uint64_t f_namelen; uint64_t f_frsize; -} ProxyStatFS; +} QEMU_PACKED ProxyStatFS; + +#define PROXY_STATFS_SZ (8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 * 2 + 8 + 8) + #endif
Replies from the virtfs proxy are made up of a fixed-size header (8 bytes) and a payload of variable size (maximum 64kb). When receiving a reply, the proxy backend first reads the whole header and then unmarshals it. If the header is okay, it then does the same operation with the payload. Since the proxy backend uses a pre-allocated buffer which has enough room for a header and the maximum payload size, marshalling should never fail with fixed size arguments. Any error here is likely to result from a more serious corruption in QEMU and we'd better dump core right away. This patch adds error checks where they are missing and converts the associated error paths into assertions. Note that we don't want to use sizeof() in the checks since the value we want to use depends on the format rather than the size of the buffer. Short marshal formats can be handled with numerical values. Size macros are introduced for bigger ones (ProxyStat and ProxyStatFS). This should also address Coverity's complaints CID 1348519 and CID 1348520, about not always checking the return value of proxy_unmarshal(). Signed-off-by: Greg Kurz <groug@kaod.org> --- v2: - added PROXY_STAT_SZ and PROXY_STATFS_SZ macros - updated changelog --- hw/9pfs/9p-proxy.c | 24 +++++++++++++----------- hw/9pfs/9p-proxy.h | 10 ++++++++-- 2 files changed, 21 insertions(+), 13 deletions(-)