[PATCH] pvcalls-front: fix potential null dereference

[PATCH] pvcalls-front: fix potential null dereference

[Wen Yang]

 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);
 	free_page((unsigned long)map->active.ring);
 }
 
-- 
2.9.5

Re: [PATCH] pvcalls-front: fix potential null dereference

[Boris Ostrovsky]

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);
>  }
>