On Tue, Jan 14, 2025 at 7:18 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > 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. Okay, There was a macro for max ipc payload size, So I have changed INT_MAX to KSMBD_IPC_MAX_PAYLOAD. I will apply it to #ksmbd-for-next-next. Thanks! > > 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;
