Re: [PATCH 1/5] sgl_alloc_order: remove 4 GiB limit

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Oct 25, 2022 at 12:46:55PM +0200, Bodo Stroesser wrote:
> On 24.10.22 19:32, Jason Gunthorpe wrote:
> > On Mon, Oct 24, 2022 at 04:32:30PM +0200, Bodo Stroesser wrote:
> > > > +struct scatterlist *sgl_alloc_order(size_t length, unsigned int order,
> > > > +				    bool chainable, gfp_t gfp, size_t *nent_p)
> > > >    {
> > > >    	struct scatterlist *sgl, *sg;
> > > >    	struct page *page;
> > > > -	unsigned int nent, nalloc;
> > > > +	size_t nent, nalloc;
> > > >    	u32 elem_len;
> > > > -	nent = round_up(length, PAGE_SIZE << order) >> (PAGE_SHIFT + order);
> > > > -	/* Check for integer overflow */
> > > > -	if (length > (nent << (PAGE_SHIFT + order)))
> > > > -		return NULL;
> > > > +	nent = length >> (PAGE_SHIFT + order);
> > > > +	if (length % (1 << (PAGE_SHIFT + order)))
> > > 
> > > This might end up doing a modulo operation for divisor 0, if caller
> > > specifies a too high order parameter, right?
> > 
> > If that happens then the first >> will be busted too and this is all
> > broken..
> > 
> > We assume the caller will pass a valid order paramter it seems, it is
> > not userspace controlled.
> > 
> 
> If a too high order is passed, alloc_pages will just return NULL, so
> in the old code sgl_alloc_order simply returns NULL. Using modulo op
> changes it to possibly crashing the system.

That is fine, we are not trying to protect against buggy callers
passing a weird order 

We are trying to protect against correct callers passing a large
length because length is user controlled or something.

Jason