diff mbox series

ksmbd: prevent some integer overflows

Message ID 205c4ec1-7c41-4f5d-8058-501fc1b5163c@moroto.mountain (mailing list archive)
State New, archived
Headers show
Series ksmbd: prevent some integer overflows | expand

Commit Message

Dan Carpenter Oct. 20, 2023, 2:07 p.m. UTC
The "payload_sz" comes from the user and it can be to U32_MAX.  On a
32bit system that could lead to integer overflows and crashing.

Fixes: 0626e6641f6b ("cifsd: add server handler for central processing and tranport layers")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 fs/smb/server/transport_ipc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Namjae Jeon Oct. 25, 2023, 1:11 p.m. UTC | #1
> @@ -757,7 +756,7 @@ struct ksmbd_rpc_command *ksmbd_rpc_ioctl(struct
Hi Dan,

> ksmbd_session *sess, int handle
>  	struct ksmbd_rpc_command *req;
>  	struct ksmbd_rpc_command *resp;
>
> -	msg = ipc_msg_alloc(sizeof(struct ksmbd_rpc_command) + payload_sz + 1);
> +	msg = ipc_msg_alloc(size_add(sizeof(struct ksmbd_rpc_command) + 1,
> payload_sz));
>  	if (!msg)
>  		return NULL;
There is a memcpy() below as follows.
 memcpy(req->payload, payload, payload_sz);

Doesn't memcpy with payload_sz cause buffer overflow?
Wouldn't it be better to handle integer overflows as an error?

Thanks.
Dan Carpenter Oct. 25, 2023, 1:42 p.m. UTC | #2
On Wed, Oct 25, 2023 at 10:11:41PM +0900, Namjae Jeon wrote:
> > @@ -757,7 +756,7 @@ struct ksmbd_rpc_command *ksmbd_rpc_ioctl(struct
> Hi Dan,
> 
> > ksmbd_session *sess, int handle
> >  	struct ksmbd_rpc_command *req;
> >  	struct ksmbd_rpc_command *resp;
> >
> > -	msg = ipc_msg_alloc(sizeof(struct ksmbd_rpc_command) + payload_sz + 1);
> > +	msg = ipc_msg_alloc(size_add(sizeof(struct ksmbd_rpc_command) + 1,
> > payload_sz));
> >  	if (!msg)
> >  		return NULL;
> There is a memcpy() below as follows.
>  memcpy(req->payload, payload, payload_sz);
> 
> Doesn't memcpy with payload_sz cause buffer overflow?
> Wouldn't it be better to handle integer overflows as an error?

In the original code, then the memcpy() is the issue I was concerned
about.  I don't think it can be easily used for privelege escalation but
it can definitly crash the system.

The danger over integer overflows is that you do some math and the size
becomes small and the allocation succeeds.  But then we do an memcpy()
with no math and the size is very large.  We're trying to memcpy() a
large thing into a small buffer and it overflows.

What size_add() does is that if there is an integer overflow then
instead of wrapping to a small number then it just gets stuck at
ULONG_MAX.  The allocation will fail.  The allocation will actually
fail pretty quickly but even if it didn't that's fine because we
optimize for the success case (and not for hackers).

The rules with size_add() are that you have to be careful with the
results.  You can't add use it for math again except through the
size_add/mul() helpers.  You also must always save the result to size_t
or unsigned long.

regards,
dan carpenter
diff mbox series

Patch

diff --git a/fs/smb/server/transport_ipc.c b/fs/smb/server/transport_ipc.c
index b49d47bdafc9..086e5ee4ebb3 100644
--- a/fs/smb/server/transport_ipc.c
+++ b/fs/smb/server/transport_ipc.c
@@ -227,9 +227,8 @@  static void ipc_update_last_active(void)
 static struct ksmbd_ipc_msg *ipc_msg_alloc(size_t sz)
 {
 	struct ksmbd_ipc_msg *msg;
-	size_t msg_sz = sz + sizeof(struct ksmbd_ipc_msg);
 
-	msg = kvzalloc(msg_sz, GFP_KERNEL);
+	msg = kvzalloc(size_add(sizeof(struct ksmbd_ipc_msg), sz), GFP_KERNEL);
 	if (msg)
 		msg->sz = sz;
 	return msg;
@@ -709,7 +708,7 @@  struct ksmbd_rpc_command *ksmbd_rpc_write(struct ksmbd_session *sess, int handle
 	struct ksmbd_rpc_command *req;
 	struct ksmbd_rpc_command *resp;
 
-	msg = ipc_msg_alloc(sizeof(struct ksmbd_rpc_command) + payload_sz + 1);
+	msg = ipc_msg_alloc(size_add(sizeof(struct ksmbd_rpc_command) + 1, payload_sz));
 	if (!msg)
 		return NULL;
 
@@ -757,7 +756,7 @@  struct ksmbd_rpc_command *ksmbd_rpc_ioctl(struct ksmbd_session *sess, int handle
 	struct ksmbd_rpc_command *req;
 	struct ksmbd_rpc_command *resp;
 
-	msg = ipc_msg_alloc(sizeof(struct ksmbd_rpc_command) + payload_sz + 1);
+	msg = ipc_msg_alloc(size_add(sizeof(struct ksmbd_rpc_command) + 1, payload_sz));
 	if (!msg)
 		return NULL;
 
@@ -782,7 +781,7 @@  struct ksmbd_rpc_command *ksmbd_rpc_rap(struct ksmbd_session *sess, void *payloa
 	struct ksmbd_rpc_command *req;
 	struct ksmbd_rpc_command *resp;
 
-	msg = ipc_msg_alloc(sizeof(struct ksmbd_rpc_command) + payload_sz + 1);
+	msg = ipc_msg_alloc(size_add(sizeof(struct ksmbd_rpc_command) + 1, payload_sz));
 	if (!msg)
 		return NULL;