diff mbox series

[v2] 9p/net: fix improper handling of bogus negative read/write replies

Message ID 20250317-9p_unsigned_rw-v2-1-9d7a94a916ae@codewreck.org (mailing list archive)
State New
Headers show
Series [v2] 9p/net: fix improper handling of bogus negative read/write replies | expand

Commit Message

Dominique Martinet March 16, 2025, 9:32 p.m. UTC 
In p9_client_write() and p9_client_read_once(), if the server
incorrectly replies with success but a negative write/read count then we
would consider written (negative) <= rsize (positive) because both
variables were signed.

Make variables unsigned to avoid this problem.

The reproducer linked below now fails with the following error instead
of a null pointer deref:
9pnet: bogus RWRITE count (4294967295 > 3)

Reported-by: Robert Morris <rtm@mit.edu>
Closes: https://lore.kernel.org/16271.1734448631@26-5-164.dynamic.csail.mit.edu
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
Changes in v2:
- fixed rsize to be u32 instead of size_t
- Link to v1: https://lore.kernel.org/r/20241222-9p_unsigned_rw-v1-1-3ea971d200cb@codewreck.org
---
 net/9p/client.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)


---
base-commit: 28e6f0643ff4431aac807e902ff0c8de16b2216d
change-id: 20241222-9p_unsigned_rw-03f95da525a0

Best regards,

Comments

Dominique Martinet March 16, 2025, 9:38 p.m. UTC | #1 
Dominique Martinet wrote on Mon, Mar 17, 2025 at 06:32:56AM +0900:
> @@ -1633,7 +1634,7 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
>  		if (count < rsize)
>  			rsize = count;
>  
> -		p9_debug(P9_DEBUG_9P, ">>> TWRITE fid %d offset %llu count %d (/%d)\n",
> +		p9_debug(P9_DEBUG_9P, ">>> TWRITE fid %d offset %llu count %zu (/%zu)\n",
>  			 fid->fid, offset, rsize, count);

I obviously ran make W=1 after sending this and noticed I forgot to
update the format fields, let's pretend this was %u for rsize (same
below)
Christian Schoenebeck March 19, 2025, 9:20 a.m. UTC | #2 
On Sunday, March 16, 2025 10:32:56 PM CET Dominique Martinet wrote:
> In p9_client_write() and p9_client_read_once(), if the server
> incorrectly replies with success but a negative write/read count then we
> would consider written (negative) <= rsize (positive) because both
> variables were signed.
> 
> Make variables unsigned to avoid this problem.
> 
> The reproducer linked below now fails with the following error instead
> of a null pointer deref:
> 9pnet: bogus RWRITE count (4294967295 > 3)
> 
> Reported-by: Robert Morris <rtm@mit.edu>
> Closes: https://lore.kernel.org/16271.1734448631@26-5-164.dynamic.csail.mit.edu
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
> Changes in v2:
> - fixed rsize to be u32 instead of size_t
> - Link to v1: https://lore.kernel.org/r/20241222-9p_unsigned_rw-v1-1-3ea971d200cb@codewreck.org
> ---
>  net/9p/client.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 09f8ced9f8bb..5e10fc174c3b 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -1548,7 +1548,8 @@ p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to,
>  	struct p9_client *clnt = fid->clnt;
>  	struct p9_req_t *req;
>  	int count = iov_iter_count(to);
> -	int rsize, received, non_zc = 0;
> +	u32 rsize, received;
> +	bool non_zc = false;
>  	char *dataptr;
>  
>  	*err = 0;
> @@ -1571,7 +1572,7 @@ p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to,
>  				       0, 11, "dqd", fid->fid,
>  				       offset, rsize);
>  	} else {
> -		non_zc = 1;
> +		non_zc = true;
>  		req = p9_client_rpc(clnt, P9_TREAD, "dqd", fid->fid, offset,
>  				    rsize);
>  	}
> @@ -1592,11 +1593,11 @@ p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to,
>  		return 0;
>  	}
>  	if (rsize < received) {
> -		pr_err("bogus RREAD count (%d > %d)\n", received, rsize);
> +		pr_err("bogus RREAD count (%u > %u)\n", received, rsize);
>  		received = rsize;
>  	}
>  
> -	p9_debug(P9_DEBUG_9P, "<<< RREAD count %d\n", received);
> +	p9_debug(P9_DEBUG_9P, "<<< RREAD count %u\n", received);
>  
>  	if (non_zc) {
>  		int n = copy_to_iter(dataptr, received, to);
> @@ -1623,9 +1624,9 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
>  	*err = 0;
>  
>  	while (iov_iter_count(from)) {
> -		int count = iov_iter_count(from);
> -		int rsize = fid->iounit;
> -		int written;
> +		size_t count = iov_iter_count(from);
> +		u32 rsize = fid->iounit;
> +		u32 written;
>  
>  		if (!rsize || rsize > clnt->msize - P9_IOHDRSZ)
>  			rsize = clnt->msize - P9_IOHDRSZ;
> @@ -1633,7 +1634,7 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
>  		if (count < rsize)
>  			rsize = count;
>  
> -		p9_debug(P9_DEBUG_9P, ">>> TWRITE fid %d offset %llu count %d (/%d)\n",
> +		p9_debug(P9_DEBUG_9P, ">>> TWRITE fid %d offset %llu count %zu (/%zu)\n",
>  			 fid->fid, offset, rsize, count);

  p9_debug(P9_DEBUG_9P, ">>> TWRITE fid %d offset %llu count %u (/%zu)\n",
           fid->fid, offset, rsize, count);

>  		/* Don't bother zerocopy for small IO (< 1024) */
> @@ -1659,11 +1660,11 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
>  			break;
>  		}
>  		if (rsize < written) {
> -			pr_err("bogus RWRITE count (%d > %d)\n", written, rsize);
> +			pr_err("bogus RWRITE count (%u > %zu)\n", written, rsize);

  pr_err("bogus RWRITE count (%u > %u)\n", written, rsize);

>  			written = rsize;
>  		}
>  
> -		p9_debug(P9_DEBUG_9P, "<<< RWRITE count %d\n", written);
> +		p9_debug(P9_DEBUG_9P, "<<< RWRITE count %u\n", written);
>  
>  		p9_req_put(clnt, req);
>  		iov_iter_revert(from, count - written - iov_iter_count(from));
> @@ -2098,7 +2099,8 @@ EXPORT_SYMBOL_GPL(p9_client_xattrcreate);
>  
>  int p9_client_readdir(struct p9_fid *fid, char *data, u32 count, u64 offset)
>  {
> -	int err, rsize, non_zc = 0;
> +	int err, non_zc = 0;
> +	u32 rsize;
>  	struct p9_client *clnt;
>  	struct p9_req_t *req;
>  	char *dataptr;

Missing change for:

  p9_debug(P9_DEBUG_9P, ">>> TREADDIR fid %d offset %llu count %u",
           fid->fid, offset, count);

> @@ -2142,11 +2144,11 @@ int p9_client_readdir(struct p9_fid *fid, char *data, u32 count, u64 offset)
>  		goto free_and_error;
>  	}
>  	if (rsize < count) {
> -		pr_err("bogus RREADDIR count (%d > %d)\n", count, rsize);
> +		pr_err("bogus RREADDIR count (%u > %u)\n", count, rsize);
>  		count = rsize;
>  	}
>  
> -	p9_debug(P9_DEBUG_9P, "<<< RREADDIR count %d\n", count);
> +	p9_debug(P9_DEBUG_9P, "<<< RREADDIR count %u\n", count);
>  
>  	if (non_zc)
>  		memmove(data, dataptr, count);
> 
> ---
> base-commit: 28e6f0643ff4431aac807e902ff0c8de16b2216d
> change-id: 20241222-9p_unsigned_rw-03f95da525a0
> 
> Best regards,
>
Dominique Martinet March 19, 2025, 9:42 a.m. UTC | #3 
Christian Schoenebeck wrote on Wed, Mar 19, 2025 at 10:20:05AM +0100:
> > -		p9_debug(P9_DEBUG_9P, ">>> TWRITE fid %d offset %llu count %d (/%d)\n",
> > +		p9_debug(P9_DEBUG_9P, ">>> TWRITE fid %d offset %llu count %zu (/%zu)\n",
> >  			 fid->fid, offset, rsize, count);
> 
>   p9_debug(P9_DEBUG_9P, ">>> TWRITE fid %d offset %llu count %u (/%zu)\n",
>            fid->fid, offset, rsize, count);
> >  		if (rsize < written) {
> > -			pr_err("bogus RWRITE count (%d > %d)\n", written, rsize);
> > +			pr_err("bogus RWRITE count (%u > %zu)\n", written, rsize);
> 
>   pr_err("bogus RWRITE count (%u > %u)\n", written, rsize);

(Right, got these two as they cause a warning with W=1)

> >  int p9_client_readdir(struct p9_fid *fid, char *data, u32 count, u64 offset)
> >  {
> > -	int err, rsize, non_zc = 0;
> > +	int err, non_zc = 0;
> > +	u32 rsize;
> >  	struct p9_client *clnt;
> >  	struct p9_req_t *req;
> >  	char *dataptr;
> 
> Missing change for:
> 
>   p9_debug(P9_DEBUG_9P, ">>> TREADDIR fid %d offset %llu count %u",
>            fid->fid, offset, count);

Oh, this one doesn't seem to cause a warning.
I guess it doesn't matter much unless we get something > 2^31 but might
as well fix, sending as a v2
diff mbox series

Patch

diff --git a/net/9p/client.c b/net/9p/client.c
index 09f8ced9f8bb..5e10fc174c3b 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -1548,7 +1548,8 @@  p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to,
 	struct p9_client *clnt = fid->clnt;
 	struct p9_req_t *req;
 	int count = iov_iter_count(to);
-	int rsize, received, non_zc = 0;
+	u32 rsize, received;
+	bool non_zc = false;
 	char *dataptr;
 
 	*err = 0;
@@ -1571,7 +1572,7 @@  p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to,
 				       0, 11, "dqd", fid->fid,
 				       offset, rsize);
 	} else {
-		non_zc = 1;
+		non_zc = true;
 		req = p9_client_rpc(clnt, P9_TREAD, "dqd", fid->fid, offset,
 				    rsize);
 	}
@@ -1592,11 +1593,11 @@  p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to,
 		return 0;
 	}
 	if (rsize < received) {
-		pr_err("bogus RREAD count (%d > %d)\n", received, rsize);
+		pr_err("bogus RREAD count (%u > %u)\n", received, rsize);
 		received = rsize;
 	}
 
-	p9_debug(P9_DEBUG_9P, "<<< RREAD count %d\n", received);
+	p9_debug(P9_DEBUG_9P, "<<< RREAD count %u\n", received);
 
 	if (non_zc) {
 		int n = copy_to_iter(dataptr, received, to);
@@ -1623,9 +1624,9 @@  p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
 	*err = 0;
 
 	while (iov_iter_count(from)) {
-		int count = iov_iter_count(from);
-		int rsize = fid->iounit;
-		int written;
+		size_t count = iov_iter_count(from);
+		u32 rsize = fid->iounit;
+		u32 written;
 
 		if (!rsize || rsize > clnt->msize - P9_IOHDRSZ)
 			rsize = clnt->msize - P9_IOHDRSZ;
@@ -1633,7 +1634,7 @@  p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
 		if (count < rsize)
 			rsize = count;
 
-		p9_debug(P9_DEBUG_9P, ">>> TWRITE fid %d offset %llu count %d (/%d)\n",
+		p9_debug(P9_DEBUG_9P, ">>> TWRITE fid %d offset %llu count %zu (/%zu)\n",
 			 fid->fid, offset, rsize, count);
 
 		/* Don't bother zerocopy for small IO (< 1024) */
@@ -1659,11 +1660,11 @@  p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
 			break;
 		}
 		if (rsize < written) {
-			pr_err("bogus RWRITE count (%d > %d)\n", written, rsize);
+			pr_err("bogus RWRITE count (%u > %zu)\n", written, rsize);
 			written = rsize;
 		}
 
-		p9_debug(P9_DEBUG_9P, "<<< RWRITE count %d\n", written);
+		p9_debug(P9_DEBUG_9P, "<<< RWRITE count %u\n", written);
 
 		p9_req_put(clnt, req);
 		iov_iter_revert(from, count - written - iov_iter_count(from));
@@ -2098,7 +2099,8 @@  EXPORT_SYMBOL_GPL(p9_client_xattrcreate);
 
 int p9_client_readdir(struct p9_fid *fid, char *data, u32 count, u64 offset)
 {
-	int err, rsize, non_zc = 0;
+	int err, non_zc = 0;
+	u32 rsize;
 	struct p9_client *clnt;
 	struct p9_req_t *req;
 	char *dataptr;
@@ -2142,11 +2144,11 @@  int p9_client_readdir(struct p9_fid *fid, char *data, u32 count, u64 offset)
 		goto free_and_error;
 	}
 	if (rsize < count) {
-		pr_err("bogus RREADDIR count (%d > %d)\n", count, rsize);
+		pr_err("bogus RREADDIR count (%u > %u)\n", count, rsize);
 		count = rsize;
 	}
 
-	p9_debug(P9_DEBUG_9P, "<<< RREADDIR count %d\n", count);
+	p9_debug(P9_DEBUG_9P, "<<< RREADDIR count %u\n", count);
 
 	if (non_zc)
 		memmove(data, dataptr, count);