On Tue, Jan 14, 2025 at 04:53:18PM +0900, Namjae Jeon wrote: > On Mon, Jan 13, 2025 at 3:17 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > > > On 32bit systems the addition operations in ipc_msg_alloc() can > > potentially overflow leading to memory corruption. Fix this using > > size_add() which will ensure that the invalid allocations do not succeed. > You previously said that memcpy overrun does not occur due to memory > allocation failure with SIZE_MAX. > > Would it be better to handle integer overflows as an error before > memory allocation? I mean we could do something like the below patch but I'd prefer to fix it this way. > And static checkers don't detect memcpy overrun by considering memory > allocation failure? How the struct_size()/array_size() kernel hardenning works is that if you pass in a too large value instead of wrapping to a small value, the math results in SIZE_MAX so the allocation will fail. We already handle allocation failures correctly so it's fine. The problem in this code is that on 32 bit systems if you chose a "sz" value which is (unsigned int)-4 then the kvzalloc() allocation will succeed but the buffer will be 4 bytes smaller than intended and the "msg->sz = sz;" assignment will corrupt memory. Anyway, here is how the patch could look like with bounds checking instead of size_add(). We could fancy it up a bit, but I don't like fancy math. regards, dan carpenter diff --git a/fs/smb/server/transport_ipc.c b/fs/smb/server/transport_ipc.c index befaf42b84cc..e1e3bfff163c 100644 --- a/fs/smb/server/transport_ipc.c +++ b/fs/smb/server/transport_ipc.c @@ -626,6 +626,9 @@ ksmbd_ipc_spnego_authen_request(const char *spnego_blob, int blob_len) struct ksmbd_spnego_authen_request *req; struct ksmbd_spnego_authen_response *resp; + if (blob_len > INT_MAX) + return NULL; + msg = ipc_msg_alloc(sizeof(struct ksmbd_spnego_authen_request) + blob_len + 1); if (!msg) @@ -805,6 +808,9 @@ struct ksmbd_rpc_command *ksmbd_rpc_write(struct ksmbd_session *sess, int handle struct ksmbd_rpc_command *req; struct ksmbd_rpc_command *resp; + if (payload_sz > INT_MAX) + return NULL; + msg = ipc_msg_alloc(sizeof(struct ksmbd_rpc_command) + payload_sz + 1); if (!msg) return NULL; @@ -853,6 +859,9 @@ struct ksmbd_rpc_command *ksmbd_rpc_ioctl(struct ksmbd_session *sess, int handle struct ksmbd_rpc_command *req; struct ksmbd_rpc_command *resp; + if (payload_sz > INT_MAX) + return NULL; + msg = ipc_msg_alloc(sizeof(struct ksmbd_rpc_command) + payload_sz + 1); if (!msg) return NULL; @@ -878,6 +887,9 @@ struct ksmbd_rpc_command *ksmbd_rpc_rap(struct ksmbd_session *sess, void *payloa struct ksmbd_rpc_command *req; struct ksmbd_rpc_command *resp; + if (payload_sz > INT_MAX) + return NULL; + msg = ipc_msg_alloc(sizeof(struct ksmbd_rpc_command) + payload_sz + 1); if (!msg) return NULL;
