diff mbox

[Precise,CVE-2015-1805] pipe: iovec: Fix memory corruption when retrying atomic copy as non-atomic

Message ID 1435612136-6680-1-git-send-email-kamal@canonical.com
State New
Headers show

Commit Message

Kamal Mostafa June 29, 2015, 9:08 p.m. UTC
From: Ben Hutchings <ben@decadent.org.uk>

pipe_iov_copy_{from,to}_user() may be tried twice with the same iovec,
the first time atomically and the second time not.  The second attempt
needs to continue from the iovec position, pipe buffer offset and
remaining length where the first attempt failed, but currently the
pipe buffer offset and remaining length are reset.  This will corrupt
the piped data (possibly also leading to an information leak between
processes) and may also corrupt kernel memory.

This was fixed upstream by commits f0d1bec9d58d ("new helper:
copy_page_from_iter()") and 637b58c2887e ("switch pipe_read() to
copy_page_to_iter()"), but those aren't suitable for stable.  This fix
for older kernel versions was made by Seth Jennings for RHEL and I
have extracted it from their update.

CVE-2015-1805

References: https://bugzilla.redhat.com/show_bug.cgi?id=1202855
Cc: stable <stable@vger.kernel.org> # 3.14 and earlier
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Kamal Mostafa <kamal@canonical.com>
---
 fs/pipe.c | 55 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 23 deletions(-)

Comments

Stefan Bader June 30, 2015, 7:17 a.m. UTC | #1

Andy Whitcroft July 1, 2015, 8:41 a.m. UTC | #2
On Mon, Jun 29, 2015 at 02:08:56PM -0700, Kamal Mostafa wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> 
> pipe_iov_copy_{from,to}_user() may be tried twice with the same iovec,
> the first time atomically and the second time not.  The second attempt
> needs to continue from the iovec position, pipe buffer offset and
> remaining length where the first attempt failed, but currently the
> pipe buffer offset and remaining length are reset.  This will corrupt
> the piped data (possibly also leading to an information leak between
> processes) and may also corrupt kernel memory.
> 
> This was fixed upstream by commits f0d1bec9d58d ("new helper:
> copy_page_from_iter()") and 637b58c2887e ("switch pipe_read() to
> copy_page_to_iter()"), but those aren't suitable for stable.  This fix
> for older kernel versions was made by Seth Jennings for RHEL and I
> have extracted it from their update.
> 
> CVE-2015-1805
> 
> References: https://bugzilla.redhat.com/show_bug.cgi?id=1202855
> Cc: stable <stable@vger.kernel.org> # 3.14 and earlier
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Kamal Mostafa <kamal@canonical.com>
> ---
>  fs/pipe.c | 55 ++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 8ca88fc..d2cbeff 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -103,25 +103,27 @@ void pipe_wait(struct pipe_inode_info *pipe)
>  }
>  
>  static int
> -pipe_iov_copy_from_user(void *to, struct iovec *iov, unsigned long len,
> -			int atomic)
> +pipe_iov_copy_from_user(void *addr, int *offset, struct iovec *iov,
> +			size_t *remaining, int atomic)
>  {
>  	unsigned long copy;
>  
> -	while (len > 0) {
> +	while (*remaining > 0) {
>  		while (!iov->iov_len)
>  			iov++;
> -		copy = min_t(unsigned long, len, iov->iov_len);
> +		copy = min_t(unsigned long, *remaining, iov->iov_len);
>  
>  		if (atomic) {
> -			if (__copy_from_user_inatomic(to, iov->iov_base, copy))
> +			if (__copy_from_user_inatomic(addr + *offset,
> +						      iov->iov_base, copy))
>  				return -EFAULT;
>  		} else {
> -			if (copy_from_user(to, iov->iov_base, copy))
> +			if (copy_from_user(addr + *offset,
> +					   iov->iov_base, copy))
>  				return -EFAULT;
>  		}
> -		to += copy;
> -		len -= copy;
> +		*offset += copy;
> +		*remaining -= copy;
>  		iov->iov_base += copy;
>  		iov->iov_len -= copy;
>  	}
> @@ -129,25 +131,27 @@ pipe_iov_copy_from_user(void *to, struct iovec *iov, unsigned long len,
>  }
>  
>  static int
> -pipe_iov_copy_to_user(struct iovec *iov, const void *from, unsigned long len,
> -		      int atomic)
> +pipe_iov_copy_to_user(struct iovec *iov, void *addr, int *offset,
> +		      size_t *remaining, int atomic)
>  {
>  	unsigned long copy;
>  
> -	while (len > 0) {
> +	while (*remaining > 0) {
>  		while (!iov->iov_len)
>  			iov++;
> -		copy = min_t(unsigned long, len, iov->iov_len);
> +		copy = min_t(unsigned long, *remaining, iov->iov_len);
>  
>  		if (atomic) {
> -			if (__copy_to_user_inatomic(iov->iov_base, from, copy))
> +			if (__copy_to_user_inatomic(iov->iov_base,
> +						    addr + *offset, copy))
>  				return -EFAULT;
>  		} else {
> -			if (copy_to_user(iov->iov_base, from, copy))
> +			if (copy_to_user(iov->iov_base,
> +					 addr + *offset, copy))
>  				return -EFAULT;
>  		}
> -		from += copy;
> -		len -= copy;
> +		*offset += copy;
> +		*remaining -= copy;
>  		iov->iov_base += copy;
>  		iov->iov_len -= copy;
>  	}
> @@ -383,7 +387,7 @@ pipe_read(struct kiocb *iocb, const struct iovec *_iov,
>  			struct pipe_buffer *buf = pipe->bufs + curbuf;
>  			const struct pipe_buf_operations *ops = buf->ops;
>  			void *addr;
> -			size_t chars = buf->len;
> +			size_t chars = buf->len, remaining;
>  			int error, atomic;
>  
>  			if (chars > total_len)
> @@ -397,9 +401,11 @@ pipe_read(struct kiocb *iocb, const struct iovec *_iov,
>  			}
>  
>  			atomic = !iov_fault_in_pages_write(iov, chars);
> +			remaining = chars;
>  redo:
>  			addr = ops->map(pipe, buf, atomic);
> -			error = pipe_iov_copy_to_user(iov, addr + buf->offset, chars, atomic);
> +			error = pipe_iov_copy_to_user(iov, addr, &buf->offset,
> +						      &remaining, atomic);
>  			ops->unmap(pipe, buf, addr);
>  			if (unlikely(error)) {
>  				/*
> @@ -414,7 +420,6 @@ redo:
>  				break;
>  			}
>  			ret += chars;
> -			buf->offset += chars;
>  			buf->len -= chars;
>  
>  			/* Was it a packet buffer? Clean up and exit */
> @@ -521,6 +526,7 @@ pipe_write(struct kiocb *iocb, const struct iovec *_iov,
>  		if (ops->can_merge && offset + chars <= PAGE_SIZE) {
>  			int error, atomic = 1;
>  			void *addr;
> +			size_t remaining = chars;
>  
>  			error = ops->confirm(pipe, buf);
>  			if (error)
> @@ -529,8 +535,8 @@ pipe_write(struct kiocb *iocb, const struct iovec *_iov,
>  			iov_fault_in_pages_read(iov, chars);
>  redo1:
>  			addr = ops->map(pipe, buf, atomic);
> -			error = pipe_iov_copy_from_user(offset + addr, iov,
> -							chars, atomic);
> +			error = pipe_iov_copy_from_user(addr, &offset, iov,
> +							&remaining, atomic);
>  			ops->unmap(pipe, buf, addr);
>  			ret = error;
>  			do_wakeup = 1;
> @@ -565,6 +571,8 @@ redo1:
>  			struct page *page = pipe->tmp_page;
>  			char *src;
>  			int error, atomic = 1;
> +			int offset = 0;
> +			size_t remaining;
>  
>  			if (!page) {
>  				page = alloc_page(GFP_HIGHUSER);
> @@ -585,14 +593,15 @@ redo1:
>  				chars = total_len;
>  
>  			iov_fault_in_pages_read(iov, chars);
> +			remaining = chars;
>  redo2:
>  			if (atomic)
>  				src = kmap_atomic(page, KM_USER0);
>  			else
>  				src = kmap(page);
>  
> -			error = pipe_iov_copy_from_user(src, iov, chars,
> -							atomic);
> +			error = pipe_iov_copy_from_user(src, &offset, iov,
> +							&remaining, atomic);
>  			if (atomic)
>  				kunmap_atomic(src, KM_USER0);
>  			else

Seems to track offset,remaining reasonably. 

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw
Kamal Mostafa July 14, 2015, 5:57 p.m. UTC | #3

diff mbox

Patch

diff --git a/fs/pipe.c b/fs/pipe.c
index 8ca88fc..d2cbeff 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -103,25 +103,27 @@  void pipe_wait(struct pipe_inode_info *pipe)
 }
 
 static int
-pipe_iov_copy_from_user(void *to, struct iovec *iov, unsigned long len,
-			int atomic)
+pipe_iov_copy_from_user(void *addr, int *offset, struct iovec *iov,
+			size_t *remaining, int atomic)
 {
 	unsigned long copy;
 
-	while (len > 0) {
+	while (*remaining > 0) {
 		while (!iov->iov_len)
 			iov++;
-		copy = min_t(unsigned long, len, iov->iov_len);
+		copy = min_t(unsigned long, *remaining, iov->iov_len);
 
 		if (atomic) {
-			if (__copy_from_user_inatomic(to, iov->iov_base, copy))
+			if (__copy_from_user_inatomic(addr + *offset,
+						      iov->iov_base, copy))
 				return -EFAULT;
 		} else {
-			if (copy_from_user(to, iov->iov_base, copy))
+			if (copy_from_user(addr + *offset,
+					   iov->iov_base, copy))
 				return -EFAULT;
 		}
-		to += copy;
-		len -= copy;
+		*offset += copy;
+		*remaining -= copy;
 		iov->iov_base += copy;
 		iov->iov_len -= copy;
 	}
@@ -129,25 +131,27 @@  pipe_iov_copy_from_user(void *to, struct iovec *iov, unsigned long len,
 }
 
 static int
-pipe_iov_copy_to_user(struct iovec *iov, const void *from, unsigned long len,
-		      int atomic)
+pipe_iov_copy_to_user(struct iovec *iov, void *addr, int *offset,
+		      size_t *remaining, int atomic)
 {
 	unsigned long copy;
 
-	while (len > 0) {
+	while (*remaining > 0) {
 		while (!iov->iov_len)
 			iov++;
-		copy = min_t(unsigned long, len, iov->iov_len);
+		copy = min_t(unsigned long, *remaining, iov->iov_len);
 
 		if (atomic) {
-			if (__copy_to_user_inatomic(iov->iov_base, from, copy))
+			if (__copy_to_user_inatomic(iov->iov_base,
+						    addr + *offset, copy))
 				return -EFAULT;
 		} else {
-			if (copy_to_user(iov->iov_base, from, copy))
+			if (copy_to_user(iov->iov_base,
+					 addr + *offset, copy))
 				return -EFAULT;
 		}
-		from += copy;
-		len -= copy;
+		*offset += copy;
+		*remaining -= copy;
 		iov->iov_base += copy;
 		iov->iov_len -= copy;
 	}
@@ -383,7 +387,7 @@  pipe_read(struct kiocb *iocb, const struct iovec *_iov,
 			struct pipe_buffer *buf = pipe->bufs + curbuf;
 			const struct pipe_buf_operations *ops = buf->ops;
 			void *addr;
-			size_t chars = buf->len;
+			size_t chars = buf->len, remaining;
 			int error, atomic;
 
 			if (chars > total_len)
@@ -397,9 +401,11 @@  pipe_read(struct kiocb *iocb, const struct iovec *_iov,
 			}
 
 			atomic = !iov_fault_in_pages_write(iov, chars);
+			remaining = chars;
 redo:
 			addr = ops->map(pipe, buf, atomic);
-			error = pipe_iov_copy_to_user(iov, addr + buf->offset, chars, atomic);
+			error = pipe_iov_copy_to_user(iov, addr, &buf->offset,
+						      &remaining, atomic);
 			ops->unmap(pipe, buf, addr);
 			if (unlikely(error)) {
 				/*
@@ -414,7 +420,6 @@  redo:
 				break;
 			}
 			ret += chars;
-			buf->offset += chars;
 			buf->len -= chars;
 
 			/* Was it a packet buffer? Clean up and exit */
@@ -521,6 +526,7 @@  pipe_write(struct kiocb *iocb, const struct iovec *_iov,
 		if (ops->can_merge && offset + chars <= PAGE_SIZE) {
 			int error, atomic = 1;
 			void *addr;
+			size_t remaining = chars;
 
 			error = ops->confirm(pipe, buf);
 			if (error)
@@ -529,8 +535,8 @@  pipe_write(struct kiocb *iocb, const struct iovec *_iov,
 			iov_fault_in_pages_read(iov, chars);
 redo1:
 			addr = ops->map(pipe, buf, atomic);
-			error = pipe_iov_copy_from_user(offset + addr, iov,
-							chars, atomic);
+			error = pipe_iov_copy_from_user(addr, &offset, iov,
+							&remaining, atomic);
 			ops->unmap(pipe, buf, addr);
 			ret = error;
 			do_wakeup = 1;
@@ -565,6 +571,8 @@  redo1:
 			struct page *page = pipe->tmp_page;
 			char *src;
 			int error, atomic = 1;
+			int offset = 0;
+			size_t remaining;
 
 			if (!page) {
 				page = alloc_page(GFP_HIGHUSER);
@@ -585,14 +593,15 @@  redo1:
 				chars = total_len;
 
 			iov_fault_in_pages_read(iov, chars);
+			remaining = chars;
 redo2:
 			if (atomic)
 				src = kmap_atomic(page, KM_USER0);
 			else
 				src = kmap(page);
 
-			error = pipe_iov_copy_from_user(src, iov, chars,
-							atomic);
+			error = pipe_iov_copy_from_user(src, &offset, iov,
+							&remaining, atomic);
 			if (atomic)
 				kunmap_atomic(src, KM_USER0);
 			else