Takuya Yoshikawa
2010-May-27 09:58 UTC
[PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly
copy_to/from_user() returns the number of bytes that could not be copied. So we need to check if it is not zero, and in that case, we should return the error number -EFAULT rather than directly return the return value from copy_to/from_user(). Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya at oss.ntt.co.jp> --- drivers/vhost/vhost.c | 51 ++++++++++++++++++++++++++---------------------- 1 files changed, 28 insertions(+), 23 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 5c9c657..9633a3c 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -320,10 +320,8 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) { struct vhost_memory mem, *newmem, *oldmem; unsigned long size = offsetof(struct vhost_memory, regions); - long r; - r = copy_from_user(&mem, m, size); - if (r) - return r; + if (copy_from_user(&mem, m, size)) + return -EFAULT; if (mem.padding) return -EOPNOTSUPP; if (mem.nregions > VHOST_MEMORY_MAX_NREGIONS) @@ -333,11 +331,10 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) return -ENOMEM; memcpy(newmem, &mem, size); - r = copy_from_user(newmem->regions, m->regions, - mem.nregions * sizeof *m->regions); - if (r) { + if (copy_from_user(newmem->regions, m->regions, + mem.nregions * sizeof *m->regions)) { kfree(newmem); - return r; + return -EFAULT; } if (!memory_access_ok(d, newmem, vhost_has_feature(d, VHOST_F_LOG_ALL))) @@ -389,9 +386,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp) r = -EBUSY; break; } - r = copy_from_user(&s, argp, sizeof s); - if (r < 0) + if (copy_from_user(&s, argp, sizeof s)) { + r = -EFAULT; break; + } if (!s.num || s.num > 0xffff || (s.num & (s.num - 1))) { r = -EINVAL; break; @@ -405,9 +403,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp) r = -EBUSY; break; } - r = copy_from_user(&s, argp, sizeof s); - if (r < 0) + if (copy_from_user(&s, argp, sizeof s)) { + r = -EFAULT; break; + } if (s.num > 0xffff) { r = -EINVAL; break; @@ -419,12 +418,14 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp) case VHOST_GET_VRING_BASE: s.index = idx; s.num = vq->last_avail_idx; - r = copy_to_user(argp, &s, sizeof s); + if (copy_to_user(argp, &s, sizeof s)) + r = -EFAULT; break; case VHOST_SET_VRING_ADDR: - r = copy_from_user(&a, argp, sizeof a); - if (r < 0) + if (copy_from_user(&a, argp, sizeof a)) { + r = -EFAULT; break; + } if (a.flags & ~(0x1 << VHOST_VRING_F_LOG)) { r = -EOPNOTSUPP; break; @@ -477,9 +478,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp) vq->used = (void __user *)(unsigned long)a.used_user_addr; break; case VHOST_SET_VRING_KICK: - r = copy_from_user(&f, argp, sizeof f); - if (r < 0) + if (copy_from_user(&f, argp, sizeof f)) { + r = -EFAULT; break; + } eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd); if (IS_ERR(eventfp)) { r = PTR_ERR(eventfp); @@ -492,9 +494,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp) filep = eventfp; break; case VHOST_SET_VRING_CALL: - r = copy_from_user(&f, argp, sizeof f); - if (r < 0) + if (copy_from_user(&f, argp, sizeof f)) { + r = -EFAULT; break; + } eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd); if (IS_ERR(eventfp)) { r = PTR_ERR(eventfp); @@ -510,9 +513,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp) filep = eventfp; break; case VHOST_SET_VRING_ERR: - r = copy_from_user(&f, argp, sizeof f); - if (r < 0) + if (copy_from_user(&f, argp, sizeof f)) { + r = -EFAULT; break; + } eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd); if (IS_ERR(eventfp)) { r = PTR_ERR(eventfp); @@ -575,9 +579,10 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg) r = vhost_set_memory(d, argp); break; case VHOST_SET_LOG_BASE: - r = copy_from_user(&p, argp, sizeof p); - if (r < 0) + if (copy_from_user(&p, argp, sizeof p)) { + r = -EFAULT; break; + } if ((u64)(unsigned long)p != p) { r = -EFAULT; break; -- 1.7.0.4
Takuya Yoshikawa
2010-May-27 10:01 UTC
[PATCH 2/3] vhost-net: fix to check the return value of copy_to/from_user() correctly
copy_to/from_user() returns the number of bytes that could not be copied. So we need to check if it is not zero, and in that case, we should return the error number -EFAULT rather than directly return the return value from copy_to/from_user(). Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya at oss.ntt.co.jp> --- drivers/vhost/net.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index aa88911..0f41c91 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -593,17 +593,17 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl, int r; switch (ioctl) { case VHOST_NET_SET_BACKEND: - r = copy_from_user(&backend, argp, sizeof backend); - if (r < 0) - return r; + if (copy_from_user(&backend, argp, sizeof backend)) + return -EFAULT; return vhost_net_set_backend(n, backend.index, backend.fd); case VHOST_GET_FEATURES: features = VHOST_FEATURES; - return copy_to_user(featurep, &features, sizeof features); + if (copy_to_user(featurep, &features, sizeof features)) + return -EFAULT; + return 0; case VHOST_SET_FEATURES: - r = copy_from_user(&features, featurep, sizeof features); - if (r < 0) - return r; + if (copy_from_user(&features, featurep, sizeof features)) + return -EFAULT; if (features & ~VHOST_FEATURES) return -EOPNOTSUPP; return vhost_net_set_features(n, features); -- 1.7.0.4
Takuya Yoshikawa
2010-May-27 10:03 UTC
[PATCH 3/3] vhost: fix the memory leak which will happen when memory_access_ok fails
We need to free newmem when vhost_set_memory() fails to complete. Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya at oss.ntt.co.jp> --- drivers/vhost/vhost.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 9633a3c..1241a22 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -337,8 +337,10 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) return -EFAULT; } - if (!memory_access_ok(d, newmem, vhost_has_feature(d, VHOST_F_LOG_ALL))) + if (!memory_access_ok(d, newmem, vhost_has_feature(d, VHOST_F_LOG_ALL))) { + kfree(newmem); return -EFAULT; + } oldmem = d->memory; rcu_assign_pointer(d->memory, newmem); synchronize_rcu(); -- 1.7.0.4
Michael S. Tsirkin
2010-May-27 10:48 UTC
[PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly
On Thu, May 27, 2010 at 06:58:03PM +0900, Takuya Yoshikawa wrote:> copy_to/from_user() returns the number of bytes that could not be copied. > > So we need to check if it is not zero, and in that case, we should return > the error number -EFAULT rather than directly return the return value from > copy_to/from_user(). > > Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya at oss.ntt.co.jp>Thanks, applied.> --- > drivers/vhost/vhost.c | 51 ++++++++++++++++++++++++++---------------------- > 1 files changed, 28 insertions(+), 23 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index 5c9c657..9633a3c 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -320,10 +320,8 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) > { > struct vhost_memory mem, *newmem, *oldmem; > unsigned long size = offsetof(struct vhost_memory, regions); > - long r; > - r = copy_from_user(&mem, m, size); > - if (r) > - return r; > + if (copy_from_user(&mem, m, size)) > + return -EFAULT; > if (mem.padding) > return -EOPNOTSUPP; > if (mem.nregions > VHOST_MEMORY_MAX_NREGIONS) > @@ -333,11 +331,10 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) > return -ENOMEM; > > memcpy(newmem, &mem, size); > - r = copy_from_user(newmem->regions, m->regions, > - mem.nregions * sizeof *m->regions); > - if (r) { > + if (copy_from_user(newmem->regions, m->regions, > + mem.nregions * sizeof *m->regions)) { > kfree(newmem); > - return r; > + return -EFAULT; > } > > if (!memory_access_ok(d, newmem, vhost_has_feature(d, VHOST_F_LOG_ALL))) > @@ -389,9 +386,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp) > r = -EBUSY; > break; > } > - r = copy_from_user(&s, argp, sizeof s); > - if (r < 0) > + if (copy_from_user(&s, argp, sizeof s)) { > + r = -EFAULT; > break; > + } > if (!s.num || s.num > 0xffff || (s.num & (s.num - 1))) { > r = -EINVAL; > break; > @@ -405,9 +403,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp) > r = -EBUSY; > break; > } > - r = copy_from_user(&s, argp, sizeof s); > - if (r < 0) > + if (copy_from_user(&s, argp, sizeof s)) { > + r = -EFAULT; > break; > + } > if (s.num > 0xffff) { > r = -EINVAL; > break; > @@ -419,12 +418,14 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp) > case VHOST_GET_VRING_BASE: > s.index = idx; > s.num = vq->last_avail_idx; > - r = copy_to_user(argp, &s, sizeof s); > + if (copy_to_user(argp, &s, sizeof s)) > + r = -EFAULT; > break; > case VHOST_SET_VRING_ADDR: > - r = copy_from_user(&a, argp, sizeof a); > - if (r < 0) > + if (copy_from_user(&a, argp, sizeof a)) { > + r = -EFAULT; > break; > + } > if (a.flags & ~(0x1 << VHOST_VRING_F_LOG)) { > r = -EOPNOTSUPP; > break; > @@ -477,9 +478,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp) > vq->used = (void __user *)(unsigned long)a.used_user_addr; > break; > case VHOST_SET_VRING_KICK: > - r = copy_from_user(&f, argp, sizeof f); > - if (r < 0) > + if (copy_from_user(&f, argp, sizeof f)) { > + r = -EFAULT; > break; > + } > eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd); > if (IS_ERR(eventfp)) { > r = PTR_ERR(eventfp); > @@ -492,9 +494,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp) > filep = eventfp; > break; > case VHOST_SET_VRING_CALL: > - r = copy_from_user(&f, argp, sizeof f); > - if (r < 0) > + if (copy_from_user(&f, argp, sizeof f)) { > + r = -EFAULT; > break; > + } > eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd); > if (IS_ERR(eventfp)) { > r = PTR_ERR(eventfp); > @@ -510,9 +513,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp) > filep = eventfp; > break; > case VHOST_SET_VRING_ERR: > - r = copy_from_user(&f, argp, sizeof f); > - if (r < 0) > + if (copy_from_user(&f, argp, sizeof f)) { > + r = -EFAULT; > break; > + } > eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd); > if (IS_ERR(eventfp)) { > r = PTR_ERR(eventfp); > @@ -575,9 +579,10 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg) > r = vhost_set_memory(d, argp); > break; > case VHOST_SET_LOG_BASE: > - r = copy_from_user(&p, argp, sizeof p); > - if (r < 0) > + if (copy_from_user(&p, argp, sizeof p)) { > + r = -EFAULT; > break; > + } > if ((u64)(unsigned long)p != p) { > r = -EFAULT; > break; > -- > 1.7.0.4
Michael S. Tsirkin
2010-May-27 10:48 UTC
[PATCH 2/3] vhost-net: fix to check the return value of copy_to/from_user() correctly
On Thu, May 27, 2010 at 07:01:58PM +0900, Takuya Yoshikawa wrote:> copy_to/from_user() returns the number of bytes that could not be copied. > > So we need to check if it is not zero, and in that case, we should return > the error number -EFAULT rather than directly return the return value from > copy_to/from_user(). > > Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya at oss.ntt.co.jp>Thanks, applied.> --- > drivers/vhost/net.c | 14 +++++++------- > 1 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c > index aa88911..0f41c91 100644 > --- a/drivers/vhost/net.c > +++ b/drivers/vhost/net.c > @@ -593,17 +593,17 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl, > int r; > switch (ioctl) { > case VHOST_NET_SET_BACKEND: > - r = copy_from_user(&backend, argp, sizeof backend); > - if (r < 0) > - return r; > + if (copy_from_user(&backend, argp, sizeof backend)) > + return -EFAULT; > return vhost_net_set_backend(n, backend.index, backend.fd); > case VHOST_GET_FEATURES: > features = VHOST_FEATURES; > - return copy_to_user(featurep, &features, sizeof features); > + if (copy_to_user(featurep, &features, sizeof features)) > + return -EFAULT; > + return 0; > case VHOST_SET_FEATURES: > - r = copy_from_user(&features, featurep, sizeof features); > - if (r < 0) > - return r; > + if (copy_from_user(&features, featurep, sizeof features)) > + return -EFAULT; > if (features & ~VHOST_FEATURES) > return -EOPNOTSUPP; > return vhost_net_set_features(n, features); > -- > 1.7.0.4
Michael S. Tsirkin
2010-May-27 10:49 UTC
[PATCH 3/3] vhost: fix the memory leak which will happen when memory_access_ok fails
On Thu, May 27, 2010 at 07:03:56PM +0900, Takuya Yoshikawa wrote:> We need to free newmem when vhost_set_memory() fails to complete. > > Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya at oss.ntt.co.jp> > ---Thanks, applied.> drivers/vhost/vhost.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index 9633a3c..1241a22 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -337,8 +337,10 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) > return -EFAULT; > } > > - if (!memory_access_ok(d, newmem, vhost_has_feature(d, VHOST_F_LOG_ALL))) > + if (!memory_access_ok(d, newmem, vhost_has_feature(d, VHOST_F_LOG_ALL))) { > + kfree(newmem); > return -EFAULT; > + } > oldmem = d->memory; > rcu_assign_pointer(d->memory, newmem); > synchronize_rcu(); > -- > 1.7.0.4