| Message ID | 57e9f7e2.453fca0a.ec394.0b36@mx.google.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 26 Sep 2016 21:38:48 -0700 Li Qiang <liq3ea@gmail.com> wrote: > From: Li Qiang <liqiang6-s@360.cn> > > In 9pfs get version dispatch function, a guest can provide a NULL > version string thus causing an NULL pointer dereference issue. The guest doesn't provide NULL, but an empty string actually. > This patch fix this issue. > > Signed-off-by: Li Qiang <liqiang6-s@360.cn> > --- > hw/9pfs/9p.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index 119ee58..dd3145c 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -955,6 +955,11 @@ static void v9fs_version(void *opaque) > offset = err; > goto out; > } > + > + if (!version.data) { > + offset = -EINVAL; > + goto out; > + } If a client sends a Tversion message with an unrecognized version (which is obviously the case with an empty string), the server should send back a Rversion response with 'unknown'... while with this patch it will send Rerror. http://man.cat-v.org/plan_9/5/version > trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data); > > virtfs_reset(pdu); All guests originated strings come from v9fs_iov_vunmarshal() actually, which does this: str->data = g_malloc(str->size + 1); copied = v9fs_unpack(str->data, out_sg, out_num, offset, str->size); if (copied > 0) { str->data[str->size] = 0; } else { v9fs_string_free(str); } It makes sense to free the buffer when v9fs_unpack() fails, because the error is propagated and the caller isn't supposed to derefence str->data in this case. But it looks wrong to do the same with an empty string, which is expected to be usable by the caller. The fix is to check copied >= 0. Cheers. -- Greg
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 119ee58..dd3145c 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -955,6 +955,11 @@ static void v9fs_version(void *opaque) offset = err; goto out; } + + if (!version.data) { + offset = -EINVAL; + goto out; + } trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data); virtfs_reset(pdu);