| Message ID | b00cd043-7e52-4462-8bb7-b067095bd5fd@stanley.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] ksmbd: fix integer overflows on 32 bit systems | expand |
On Mon, Jan 13, 2025 at 3:17 PM Dan Carpenter <dan.carpenter@linaro.org> 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? And static checkers don't detect memcpy overrun by considering memory allocation failure? Thanks. > In the callers, move the two constant values > "sizeof(struct ksmbd_rpc_command) + 1" onto the same side and use > size_add() for the user controlled values. > > Fixes: 0626e6641f6b ("cifsd: add server handler for central processing and tranport layers") > Cc: stable@vger.kernel.org > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > I sent this patch in Oct 2023 but it wasn't applied. > https://lore.kernel.org/all/205c4ec1-7c41-4f5d-8058-501fc1b5163c@moroto.mountain/ > I reviewed this code again today and it is still an issue. > > fs/smb/server/transport_ipc.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/fs/smb/server/transport_ipc.c b/fs/smb/server/transport_ipc.c > index befaf42b84cc..ec72c97b2f0b 100644 > --- a/fs/smb/server/transport_ipc.c > +++ b/fs/smb/server/transport_ipc.c > @@ -242,7 +242,7 @@ 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); > + size_t msg_sz = size_add(sz, sizeof(struct ksmbd_ipc_msg)); > > msg = kvzalloc(msg_sz, KSMBD_DEFAULT_GFP); > if (msg) > @@ -626,8 +626,8 @@ ksmbd_ipc_spnego_authen_request(const char *spnego_blob, int blob_len) > struct ksmbd_spnego_authen_request *req; > struct ksmbd_spnego_authen_response *resp; > > - msg = ipc_msg_alloc(sizeof(struct ksmbd_spnego_authen_request) + > - blob_len + 1); > + msg = ipc_msg_alloc(size_add(sizeof(struct ksmbd_spnego_authen_request) + 1, > + blob_len)); > if (!msg) > return NULL; > > @@ -805,7 +805,8 @@ 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; > > @@ -853,7 +854,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; > > @@ -878,7 +879,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; > > -- > 2.45.2 >
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@linaro.org> 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;
On Tue, Jan 14, 2025 at 7:18 PM Dan Carpenter <dan.carpenter@linaro.org> 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@linaro.org> 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;
On Wed, Jan 15, 2025 at 09:20:54AM +0900, Namjae Jeon wrote: > On Tue, Jan 14, 2025 at 7:18 PM Dan Carpenter <dan.carpenter@linaro.org> 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@linaro.org> 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. Nice. I didn't know. Thanks! regards, dan carpenter
diff --git a/fs/smb/server/transport_ipc.c b/fs/smb/server/transport_ipc.c index befaf42b84cc..ec72c97b2f0b 100644 --- a/fs/smb/server/transport_ipc.c +++ b/fs/smb/server/transport_ipc.c @@ -242,7 +242,7 @@ 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); + size_t msg_sz = size_add(sz, sizeof(struct ksmbd_ipc_msg)); msg = kvzalloc(msg_sz, KSMBD_DEFAULT_GFP); if (msg) @@ -626,8 +626,8 @@ ksmbd_ipc_spnego_authen_request(const char *spnego_blob, int blob_len) struct ksmbd_spnego_authen_request *req; struct ksmbd_spnego_authen_response *resp; - msg = ipc_msg_alloc(sizeof(struct ksmbd_spnego_authen_request) + - blob_len + 1); + msg = ipc_msg_alloc(size_add(sizeof(struct ksmbd_spnego_authen_request) + 1, + blob_len)); if (!msg) return NULL; @@ -805,7 +805,8 @@ 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; @@ -853,7 +854,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; @@ -878,7 +879,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;
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. In the callers, move the two constant values "sizeof(struct ksmbd_rpc_command) + 1" onto the same side and use size_add() for the user controlled values. Fixes: 0626e6641f6b ("cifsd: add server handler for central processing and tranport layers") Cc: stable@vger.kernel.org Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- I sent this patch in Oct 2023 but it wasn't applied. https://lore.kernel.org/all/205c4ec1-7c41-4f5d-8058-501fc1b5163c@moroto.mountain/ I reviewed this code again today and it is still an issue. fs/smb/server/transport_ipc.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)