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