Skip to contentSkip to conversation

9pfs: fix NULL pointer dereference in v9fs_version

PATCH In review Li Qiang 2 participants 2 messages Sep 27, 2016

lore
Share & tools
lore

Li Qiang

author
#0 lore raw

PATCH 1/1 9pfs: fix NULL pointer dereference in v9fs_version

From: Li Qiang <[email protected]>

In 9pfs get version dispatch function, a guest can provide a NULL version string thus causing an NULL pointer dereference issue. This patch fix this issue.

Signed-off-by Li Qiang <[email protected]>
---
 hw/9pfs/9p.c | 5 +++++
 1 file changed, 5 insertions(+)
hw/9pfs/9p.c +5 -0
index 119ee58..dd3145c 100644
@@ -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);
Signature · 1 line
1.8.3.1
#1 lore raw
Quoted 4 lines · On Mon, 26 Sep 2016 21:38:48 -0700 Li Qiang <[email protected]> wrote:

From: Li Qiang <[email protected]>

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.

Quoted 12 lines

This patch fix this issue.

Signed-off-by Li Qiang <[email protected]>
---
 hw/9pfs/9p.c | 5 +++++
 1 file changed, 5 insertions(+)
hw/9pfs/9p.c +0 -0
index 119ee58..dd3145c 100644

hw/9pfs/9p.c static void v9fs_version(void *opaque) · line 962

Quoted 8 lines
hw/9pfs/9p.c +5 -0
@@ -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

hw/9pfs/9p.c static void v9fs_version(void *opaque) · line 965

Quoted 3 lines
hw/9pfs/9p.c +0 -0
     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