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
