[bug report] cxl: Add guest-specific code

[bug report] cxl: Add guest-specific code

[Dan Carpenter]

Hello Christophe Lombard,

The patch 14baf4d9c739: "cxl: Add guest-specific code" from Mar 4,
2016, leads to the following static checker warning:

	drivers/misc/cxl/guest.c:1115 cxl_guest_init_adapter()
	error: we previously assumed 'adapter->guest' could be null (see line 1114)

drivers/misc/cxl/guest.c
  1105  struct cxl *cxl_guest_init_adapter(struct device_node *np, struct platform_device *pdev)
  1106  {
  1107          struct cxl *adapter;
  1108          bool free = true;
  1109          int rc;
  1110  
  1111          if (!(adapter = cxl_alloc_adapter()))
  1112                  return ERR_PTR(-ENOMEM);
  1113  
  1114          if (!(adapter->guest = kzalloc(sizeof(struct cxl_guest), GFP_KERNEL))) {
  1115                  free_adapter(adapter);
                        ^^^^^^^^^^^^^^^^^^^^^
We can't call free_adapter() if adapter->guest is NULL.

  1116                  return ERR_PTR(-ENOMEM);
  1117          }


	drivers/misc/cxl/guest.c:919 afu_properties_look_ok()
	warn: unsigned 'afu->crs_len' is never less than zero.

drivers/misc/cxl/guest.c
   907  static int afu_properties_look_ok(struct cxl_afu *afu)
   908  {
   909          if (afu->pp_irqs < 0) {
   910                  dev_err(&afu->dev, "Unexpected per-process minimum interrupt value\n");
   911                  return -EINVAL;
   912          }
   913  
   914          if (afu->max_procs_virtualised < 1) {
   915                  dev_err(&afu->dev, "Unexpected max number of processes virtualised value\n");
   916                  return -EINVAL;
   917          }
   918  
   919          if (afu->crs_len < 0) {
                    ^^^^^^^^^^^^^^^^
Remove this test.  Unsigned is never less than zero.

   920                  dev_err(&afu->dev, "Unexpected configuration record size value\n");
   921                  return -EINVAL;
   922          }
   923  
   924          return 0;
   925  }


regards,
dan carpenter

[PATCH] cxl: fix potential NULL dereference in free_adapter()

[Andrew Donnellan]

If kzalloc() fails when allocating adapter->guest in
cxl_guest_init_adapter(), we call free_adapter() before erroring out.
free_adapter() in turn attempts to dereference adapter->guest, which in
this case is NULL.

In free_adapter(), skip the adapter->guest cleanup if adapter->guest is
NULL.

Fixes: 14baf4d9c739 ("cxl: Add guest-specific code")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

---

Build tested only.
---
 drivers/misc/cxl/guest.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
index ee7148e..9aa58a7 100644
--- a/drivers/misc/cxl/guest.c
+++ b/drivers/misc/cxl/guest.c
@@ -1055,16 +1055,18 @@ static void free_adapter(struct cxl *adapter)
 	struct irq_avail *cur;
 	int i;
 
-	if (adapter->guest->irq_avail) {
-		for (i = 0; i < adapter->guest->irq_nranges; i++) {
-			cur = &adapter->guest->irq_avail[i];
-			kfree(cur->bitmap);
+	if (adapter->guest) {
+		if (adapter->guest->irq_avail) {
+			for (i = 0; i < adapter->guest->irq_nranges; i++) {
+				cur = &adapter->guest->irq_avail[i];
+				kfree(cur->bitmap);
+			}
+			kfree(adapter->guest->irq_avail);
 		}
-		kfree(adapter->guest->irq_avail);
+		kfree(adapter->guest->status);
+		kfree(adapter->guest);
 	}
-	kfree(adapter->guest->status);
 	cxl_remove_adapter_nr(adapter);
-	kfree(adapter->guest);
 	kfree(adapter);
 }
 
-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [bug report] cxl: Add guest-specific code

[Andrew Donnellan]

On 15/07/16 07:53, Dan Carpenter wrote:
>    919          if (afu->crs_len < 0) {
>                     ^^^^^^^^^^^^^^^^
> Remove this test.  Unsigned is never less than zero.

Is there another lower bound that we should be checking against here?

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: cxl: fix potential NULL dereference in free_adapter()

[Michael Ellerman]

On Fri, 2016-15-07 at 07:20:36 UTC, Andrew Donnellan wrote:
> If kzalloc() fails when allocating adapter->guest in
> cxl_guest_init_adapter(), we call free_adapter() before erroring out.
> free_adapter() in turn attempts to dereference adapter->guest, which in
> this case is NULL.
> 
> In free_adapter(), skip the adapter->guest cleanup if adapter->guest is
> NULL.
> 
> Fixes: 14baf4d9c739 ("cxl: Add guest-specific code")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/8fbaa51d43ef2c6a72849ec340

cheers