Messages in this thread | | | Subject | Re: [PATCH] pvcalls-front: fix potential null dereference | From | Boris Ostrovsky <> | Date | Mon, 14 Jan 2019 11:16:26 -0500 |
| |
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); > } >
| |