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@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. Nice. I didn't know. Thanks! regards, dan carpenter
