| Message ID | 57ea5b9f.87941c0a.d0b87.d5b6@mx.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 27 Sep 2016 04:44:11 -0700 Li Qiang <liq3ea@gmail.com> wrote: > From: Li Qiang <liqiang6-s@360.cn> > > In 9pfs function v9fs_iov_vunmarshal, it will not allocate space > for empty string. This will cause several NULL pointer dereference > issues. this patch fix this issue. > > Signed-off-by: Li Qiang <liqiang6-s@360.cn> > --- Talking about robustness was appropriate for your previous patches, but it does not really apply here since v9fs_iov_vunmarshal() does not have any issue with empty strings actually. I've changed the title to: 9pfs: allocate space for guest originated empty strings And while here, I've updated the changelog to provide a more detailed justification: If a guest sends an empty string paramater to any 9P operation, the current code unmarshals it into a V9fsString equal to { .size = 0, .data = NULL }. This is unfortunate because it can cause NULL pointer dereference to happen at various locations in the 9pfs code. And we don't want to check str->data everywhere we pass it to strcmp() or any other function which expects a dereferenceable pointer. This patch enforces the allocation of genuine C empty strings instead, so callers don't have to bother. Thanks. -- Greg > fsdev/9p-iov-marshal.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c > index 663cad5..1d16f8d 100644 > --- a/fsdev/9p-iov-marshal.c > +++ b/fsdev/9p-iov-marshal.c > @@ -125,7 +125,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset, > str->data = g_malloc(str->size + 1); > copied = v9fs_unpack(str->data, out_sg, out_num, offset, > str->size); > - if (copied > 0) { > + if (copied >= 0) { > str->data[str->size] = 0; > } else { > v9fs_string_free(str);
On 2016-09-28 0:40 GMT+08:00 Greg Kurz <groug@kaod.org> wrote: > > Talking about robustness was appropriate for your previous patches, but > it does not really apply here since v9fs_iov_vunmarshal() does not have > any issue with empty strings actually. > > I've changed the title to: > > 9pfs: allocate space for guest originated empty strings > > And while here, I've updated the changelog to provide a more detailed > justification: > > ... Thanks very much to point out the mistakes, I will do more next time. BTW, need I resend this patch formally? Thanks.
On Wed, 28 Sep 2016 10:14:00 +0800 李强 <liq3ea@gmail.com> wrote: > On 2016-09-28 0:40 GMT+08:00 Greg Kurz <groug@kaod.org> wrote: > > > > > Talking about robustness was appropriate for your previous patches, but > > it does not really apply here since v9fs_iov_vunmarshal() does not have > > any issue with empty strings actually. > > > > I've changed the title to: > > > > 9pfs: allocate space for guest originated empty strings > > > > And while here, I've updated the changelog to provide a more detailed > > justification: > > > > ... > > Thanks very much to point out the mistakes, I will do more next time. > > BTW, need I resend this patch formally? > No, that's ok. > Thanks.
diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c index 663cad5..1d16f8d 100644 --- a/fsdev/9p-iov-marshal.c +++ b/fsdev/9p-iov-marshal.c @@ -125,7 +125,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset, str->data = g_malloc(str->size + 1); copied = v9fs_unpack(str->data, out_sg, out_num, offset, str->size); - if (copied > 0) { + if (copied >= 0) { str->data[str->size] = 0; } else { v9fs_string_free(str);