lkml.org 
[lkml]   [2019]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] pvcalls-front: fix potential null dereference
From
Date
On 1/13/19 10:20 PM, Wen Yang wrote:
> static checker warning:
> drivers/xen/pvcalls-front.c:373 alloc_active_ring()
> error: we previously assumed 'map->active.ring' could be null
> (see line 357)
>
> drivers/xen/pvcalls-front.c
> 351 static int alloc_active_ring(struct sock_mapping *map)
> 352 {
> 353 void *bytes;
> 354
> 355 map->active.ring = (struct pvcalls_data_intf *)
> 356 get_zeroed_page(GFP_KERNEL);
> 357 if (!map->active.ring)
> ^^^^^^^^^^^^^^^^^
> Check
>
> 358 goto out;
> 359
> 360 map->active.ring->ring_order = PVCALLS_RING_ORDER;
> 361 bytes = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> 362 PVCALLS_RING_ORDER);
> 363 if (!bytes)
> 364 goto out;
> 365
> 366 map->active.data.in = bytes;
> 367 map->active.data.out = bytes +
> 368 XEN_FLEX_RING_SIZE(PVCALLS_RING_ORDER);
> 369
> 370 return 0;
> 371
> 372 out:
> --> 373 free_active_ring(map);
> ^^^
> Replace map->active.ring->ring_order with PVCALLS_RING_ORDER
> to avoid potential null dereference.
>
> Fixes: 9f51c05dc41a ("pvcalls-front: Avoid get_free_pages(GFP_KERNEL)
> under spinlock")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Dan Carpenter <dan.carpenter@oracle.com>
> CC: xen-devel@lists.xenproject.org
> CC: linux-kernel@vger.kernel.org
> ---
> drivers/xen/pvcalls-front.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> index 307861f..e56f9a3 100644
> --- a/drivers/xen/pvcalls-front.c
> +++ b/drivers/xen/pvcalls-front.c
> @@ -344,7 +344,7 @@ int pvcalls_front_socket(struct socket *sock)
> static void free_active_ring(struct sock_mapping *map)
> {
> free_pages((unsigned long)map->active.data.in,
> - map->active.ring->ring_order);
> + PVCALLS_RING_ORDER);

I would prefer to have a NULL check on map->active.ring and return
immediately if it is.

Otherwise this code assumes that map->active.ring->ring_order is always
PVCALLS_RING_ORDER, and I don't know whether we want to make this
assumption (even if this is true now).

-boris

> free_page((unsigned long)map->active.ring);
> }
>

\
 
 \ /
  Last update: 2019-01-14 17:18    [W:0.046 / U:0.588 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site