* [PATCH] 9p/net: fix improper handling of bogus negative read/write replies
@ 2024-12-22 13:57 Dominique Martinet
2024-12-22 15:29 ` Christian Schoenebeck
0 siblings, 1 reply; 3+ messages in thread
From: Dominique Martinet @ 2024-12-22 13:57 UTC (permalink / raw)
To: Eric Van Hensbergen, Latchesar Ionkov, Christian Schoenebeck
Cc: v9fs, linux-kernel, Robert Morris, Dominique Martinet
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>
---
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;
}
- 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;
+ 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,
--
Dominique Martinet | Asmadeus
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] 9p/net: fix improper handling of bogus negative read/write replies
2024-12-22 13:57 [PATCH] 9p/net: fix improper handling of bogus negative read/write replies Dominique Martinet
@ 2024-12-22 15:29 ` Christian Schoenebeck
2025-03-16 21:37 ` Dominique Martinet
0 siblings, 1 reply; 3+ messages in thread
From: Christian Schoenebeck @ 2024-12-22 15:29 UTC (permalink / raw)
To: Eric Van Hensbergen, Latchesar Ionkov, Dominique Martinet
Cc: v9fs, linux-kernel, Robert Morris, Dominique Martinet
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@mit.edu>
> Closes: https://lore.kernel.org/16271.1734448631@26-5-164.dynamic.csail.mit.edu
> Signed-off-by: Dominique Martinet <asmadeus@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..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,
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] 9p/net: fix improper handling of bogus negative read/write replies
2024-12-22 15:29 ` Christian Schoenebeck
@ 2025-03-16 21:37 ` Dominique Martinet
0 siblings, 0 replies; 3+ messages in thread
From: Dominique Martinet @ 2025-03-16 21:37 UTC (permalink / raw)
To: Christian Schoenebeck
Cc: Eric Van Hensbergen, Latchesar Ionkov, v9fs, linux-kernel,
Robert Morris
Christian Schoenebeck wrote on Sun, Dec 22, 2024 at 04:29:58PM +0100:
> > @@ -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.
Good point.. We're not setting *err here so this isn't an error (and
thus not ignored), so I guess it sort of makes sense if you look at it
from a "best effort" point of view, but I agree a bogus reply probably
better return an error.
OTOH it's a change of behaviour so I'd rather have it in another patch,
will send separately.
> > @@ -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.
Good catch, thanks!
--
Dominique Martinet | Asmadeus
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-03-16 21:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-22 13:57 [PATCH] 9p/net: fix improper handling of bogus negative read/write replies Dominique Martinet
2024-12-22 15:29 ` Christian Schoenebeck
2025-03-16 21:37 ` Dominique Martinet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox