Re: [PATCH net] xfrm: Rewrite key length conversion to avoid overflows

[Dan Carpenter <dan.carpenter@linaro.org>]

On Wed, Dec 18, 2024 at 05:42:35PM +0800, Herbert Xu wrote:
> On Tue, Dec 17, 2024 at 03:32:31PM +0300, Dan Carpenter wrote:
> >
> > That seems like basic algebra but we have a long history of getting
> > integer overflow checks wrong so these days I like to just use
> > INT_MAX where ever I can.  I wanted to use USHRT_MAX. We aren't allowed
> > to use more than USHRT_MAX bytes, but maybe we're allowed USHRT_MAX
> > bits, so I didn't do that.
> 
> There is no reason for this to overflow if we rewrite it do do
> the division carefully.  Something like this:
> 

I like it!  So obvious in retrospect.  Kees, Justin, this is probably a
good strategy for dealing with round_up() related integer overflows
generally.

overflows to zero:	(len + 7) / 8
      no overflow:	len / 8 + !!(len & 7)

> Steffen, this raises a new question: Can normal users create socket
> policies of arbtirarily long key lengths? If so we probably should
> look into limiting the key length to a sane value.  Of course, given
> namespaces we probably should do that in any case.

The length is capped in verify_one_alg() type functions:

	if (nla_len(rt) < (int)xfrm_alg_len(algp)) {

nla_len() is a USHRT_MAX so the rounded value can't be higher than that.

The (int) cast is unnecessary and confusing.  The condition should
probably flipped around so the untrusted part is on the left.

	if (xfrm_alg_len(algp) > nla_len(rt))
		return -EINVAL;

regards,
dan carpenter