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

[Christian Schoenebeck]

On Sunday, December 22, 2024 2:57:48 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@xxxxxxx>
> Closes: https://lore.kernel.org/16271.1734448631@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Dominique Martinet <asmadeus@xxxxxxxxxxxxx>
> ---
>  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..c290d96e1bb3 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;

Does `received = rsize` make sense here? I would rather do `received = 0` to
prevent copying garbage below, that would be ignored by caller on error case
anyway.

>  	}
>  
> -	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);
> +		size_t rsize = fid->iounit;

I think that would break 64-bit big-endian systems, as `rsize` is passed via
format below as "d" (32-bit) type.

> +		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);
> 
> ---
> base-commit: cdd30ebb1b9f36159d66f088b61aee264e649d7a
> change-id: 20241222-9p_unsigned_rw-03f95da525a0
> 
> Best regards,
>