| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4171850.H1WhmIdAfj@silver>
Date: Sun, 22 Dec 2024 16:29:58 +0100
From: Christian Schoenebeck <linux_oss@...debyte.com>
To: Eric Van Hensbergen <ericvh@...nel.org>,
Latchesar Ionkov <lucho@...kov.net>,
Dominique Martinet <asmadeus@...ewreck.org>
Cc: v9fs@...ts.linux.dev, linux-kernel@...r.kernel.org,
Robert Morris <rtm@....edu>, Dominique Martinet <asmadeus@...ewreck.org>
Subject:
Re: [PATCH] 9p/net: fix improper handling of bogus negative read/write
replies
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@....edu>
> Closes: https://lore.kernel.org/16271.1734448631@26-5-164.dynamic.csail.mit.edu
> Signed-off-by: Dominique Martinet <asmadeus@...ewreck.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..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,
>
Powered by blists - more mailing lists