* [PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly
@ 2010-05-27 9:58 Takuya Yoshikawa
2010-05-27 10:01 ` [PATCH 2/3] vhost-net: " Takuya Yoshikawa
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Takuya Yoshikawa @ 2010-05-27 9:58 UTC (permalink / raw)
To: mst; +Cc: kvm, virtualization, netdev
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@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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] vhost-net: fix to check the return value of copy_to/from_user() correctly
2010-05-27 9:58 [PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly Takuya Yoshikawa
@ 2010-05-27 10:01 ` Takuya Yoshikawa
2010-05-27 10:48 ` Michael S. Tsirkin
2010-05-27 10:03 ` [PATCH 3/3] vhost: fix the memory leak which will happen when memory_access_ok fails Takuya Yoshikawa
2010-05-27 10:48 ` [PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly Michael S. Tsirkin
2 siblings, 1 reply; 6+ messages in thread
From: Takuya Yoshikawa @ 2010-05-27 10:01 UTC (permalink / raw)
To: mst; +Cc: kvm, virtualization, netdev
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@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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] vhost: fix the memory leak which will happen when memory_access_ok fails
2010-05-27 9:58 [PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly Takuya Yoshikawa
2010-05-27 10:01 ` [PATCH 2/3] vhost-net: " Takuya Yoshikawa
@ 2010-05-27 10:03 ` Takuya Yoshikawa
2010-05-27 10:49 ` Michael S. Tsirkin
2010-05-27 10:48 ` [PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly Michael S. Tsirkin
2 siblings, 1 reply; 6+ messages in thread
From: Takuya Yoshikawa @ 2010-05-27 10:03 UTC (permalink / raw)
To: mst; +Cc: kvm, virtualization, netdev
We need to free newmem when vhost_set_memory() fails to complete.
Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly
2010-05-27 9:58 [PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly Takuya Yoshikawa
2010-05-27 10:01 ` [PATCH 2/3] vhost-net: " Takuya Yoshikawa
2010-05-27 10:03 ` [PATCH 3/3] vhost: fix the memory leak which will happen when memory_access_ok fails Takuya Yoshikawa
@ 2010-05-27 10:48 ` Michael S. Tsirkin
2 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2010-05-27 10:48 UTC (permalink / raw)
To: Takuya Yoshikawa; +Cc: kvm, virtualization, netdev
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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] vhost-net: fix to check the return value of copy_to/from_user() correctly
2010-05-27 10:01 ` [PATCH 2/3] vhost-net: " Takuya Yoshikawa
@ 2010-05-27 10:48 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2010-05-27 10:48 UTC (permalink / raw)
To: Takuya Yoshikawa; +Cc: kvm, virtualization, netdev
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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] vhost: fix the memory leak which will happen when memory_access_ok fails
2010-05-27 10:03 ` [PATCH 3/3] vhost: fix the memory leak which will happen when memory_access_ok fails Takuya Yoshikawa
@ 2010-05-27 10:49 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2010-05-27 10:49 UTC (permalink / raw)
To: Takuya Yoshikawa; +Cc: kvm, virtualization, netdev
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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-05-27 10:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-27 9:58 [PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly Takuya Yoshikawa
2010-05-27 10:01 ` [PATCH 2/3] vhost-net: " Takuya Yoshikawa
2010-05-27 10:48 ` Michael S. Tsirkin
2010-05-27 10:03 ` [PATCH 3/3] vhost: fix the memory leak which will happen when memory_access_ok fails Takuya Yoshikawa
2010-05-27 10:49 ` Michael S. Tsirkin
2010-05-27 10:48 ` [PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).