Re: [PATCH] ksmbd: prevent some integer overflows

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux