Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753938AbcKKKTX (ORCPT ); Fri, 11 Nov 2016 05:19:23 -0500 Received: from mail.fireflyinternet.com ([109.228.58.192]:60345 "EHLO fireflyinternet.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750825AbcKKKTV (ORCPT ); Fri, 11 Nov 2016 05:19:21 -0500 X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.65.138; Date: Fri, 11 Nov 2016 10:19:16 +0000 From: Chris Wilson To: Tvrtko Ursulin Cc: Intel-gfx@lists.freedesktop.org, Masahiro Yamada , linux-kernel@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH 2/4] lib/scatterlist: Avoid potential scatterlist entry overflow Message-ID: <20161111101916.GQ9300@nuc-i3427.alporthouse.com> Mail-Followup-To: Chris Wilson , Tvrtko Ursulin , Intel-gfx@lists.freedesktop.org, Masahiro Yamada , linux-kernel@vger.kernel.org References: <1478854220-3255-1-git-send-email-tvrtko.ursulin@linux.intel.com> <1478854220-3255-3-git-send-email-tvrtko.ursulin@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1478854220-3255-3-git-send-email-tvrtko.ursulin@linux.intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2847 Lines: 89 On Fri, Nov 11, 2016 at 08:50:18AM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > Since the scatterlist length field is an unsigned int, make > sure that sg_alloc_table_from_pages does not overflow it while > coallescing pages to a single entry. > > v2: Drop reference to future use. Use UINT_MAX. > > Signed-off-by: Tvrtko Ursulin > Cc: Masahiro Yamada > Cc: linux-kernel@vger.kernel.org > --- > lib/scatterlist.c | 25 +++++++++++++++++++------ > 1 file changed, 19 insertions(+), 6 deletions(-) > > diff --git a/lib/scatterlist.c b/lib/scatterlist.c > index e05e7fc98892..de15f369b317 100644 > --- a/lib/scatterlist.c > +++ b/lib/scatterlist.c > @@ -394,7 +394,8 @@ int sg_alloc_table_from_pages(struct sg_table *sgt, > unsigned int offset, unsigned long size, > gfp_t gfp_mask) > { > - unsigned int chunks; > + const unsigned int max_segment = UINT_MAX; > + unsigned int seg_len, chunks; > unsigned int i; > unsigned int cur_page; > int ret; > @@ -402,9 +403,16 @@ int sg_alloc_table_from_pages(struct sg_table *sgt, > > /* compute number of contiguous chunks */ > chunks = 1; > - for (i = 1; i < n_pages; ++i) > - if (page_to_pfn(pages[i]) != page_to_pfn(pages[i - 1]) + 1) > + seg_len = PAGE_SIZE; > + for (i = 1; i < n_pages; ++i) { > + if (seg_len >= max_segment || > + page_to_pfn(pages[i]) != page_to_pfn(pages[i - 1]) + 1) { > ++chunks; > + seg_len = PAGE_SIZE; > + } else { > + seg_len += PAGE_SIZE; > + } > + } > > ret = sg_alloc_table(sgt, chunks, gfp_mask); > if (unlikely(ret)) > @@ -413,17 +421,22 @@ int sg_alloc_table_from_pages(struct sg_table *sgt, > /* merging chunks and putting them into the scatterlist */ > cur_page = 0; > for_each_sg(sgt->sgl, s, sgt->orig_nents, i) { > - unsigned long chunk_size; > + unsigned int chunk_size; > unsigned int j; > > /* look for the end of the current chunk */ > + seg_len = PAGE_SIZE; > for (j = cur_page + 1; j < n_pages; ++j) > - if (page_to_pfn(pages[j]) != > + if (seg_len >= max_segment || > + page_to_pfn(pages[j]) != > page_to_pfn(pages[j - 1]) + 1) > break; > + else > + seg_len += PAGE_SIZE; > > chunk_size = ((j - cur_page) << PAGE_SHIFT) - offset; chunk_size = seg_len - offset; ? > - sg_set_page(s, pages[cur_page], min(size, chunk_size), offset); > + sg_set_page(s, pages[cur_page], > + min_t(unsigned long, size, chunk_size), offset); > size -= chunk_size; > offset = 0; > cur_page = j; Could BUG_ON(size); afterwards and BUG_ON(!size) inside? Although that requires the caller didn't make a mistake in n_pages vs offset+size. Minor comments, Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre