thr3ads.net - Virtualization - [PATCH 1/3] vhost: fix to check the return value of copy_to/from

If this information is useful, please help other people find it:
Share via:

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

head link

[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

head link

[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

head link

[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

head link

[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

head link

[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

Virtualization - May 2010 - [PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly

[PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly

[PATCH 2/3] vhost-net: fix to check the return value of copy_to/from_user() correctly

[PATCH 3/3] vhost: fix the memory leak which will happen when memory_access_ok fails

[PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly

[PATCH 2/3] vhost-net: fix to check the return value of copy_to/from_user() correctly

[PATCH 3/3] vhost: fix the memory leak which will happen when memory_access_ok fails