re: IB/usnic: Add Cisco VIC low-level hardware driver

re: IB/usnic: Add Cisco VIC low-level hardware driver

[Dan Carpenter]

Hello Upinder Malhi,

The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following static checker
warning:
	drivers/infiniband/hw/usnic/usnic_ib_verbs.c:114
	usnic_ib_fill_create_qp_resp()
	warn: check that 'resp' doesn't leak information (struct has
	a hole after 'transport')

drivers/infiniband/hw/usnic/usnic_ib_verbs.c
   109          WARN_ON(chunk->type != USNIC_VNIC_RES_TYPE_CQ);
   110          resp.cq_cnt = chunk->cnt;
   111          for (i = 0; i < chunk->cnt; i++)
   112                  resp.cq_idx[i] = chunk->res[i]->vnic_idx;
   113  
   114          err = ib_copy_to_udata(udata, &resp, sizeof(resp));
                                              ^^^^^
The "resp" struct has a struct hole and uninitialized struct members so
it leaks uninitialized stack information to the user (information
disclosure security bug).

   115          if (err) {
   116                  usnic_err("Failed to copy udata for %s", us_ibdev->ib_dev.name);
   117                  return err;
   118          }

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

re: IB/usnic: Add Cisco VIC low-level hardware driver

[Dan Carpenter]

Hello Upinder Malhi,

The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following Smatch
warning:
	drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c:467
	usnic_ib_qp_grp_create()
	error: scheduling with locks held: 'spin_lock:lock'

drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c
   449          BUG_ON(!spin_is_locked(&vf->lock));
                        ^^^^^^^^^^^^^^^^^^^^^^^^
Holding lock.
Don't call BUG_ON(), use WARN_ON()?

   450  
   451          err = usnic_vnic_res_spec_satisfied(&min_transport_spec[transport],
   452                                                  res_spec);
   453          if (err) {
   454                  usnic_err("Spec does not meet miniumum req for transport %d\n",
   455                                  transport);
   456                  log_spec(res_spec);
   457                  return ERR_PTR(err);
   458          }
   459  
   460          port_num = usnic_transport_rsrv_port(transport, 0);
   461          if (!port_num) {
   462                  usnic_err("Unable to allocate port for %s\n",
   463                                  netdev_name(ufdev->netdev));
   464                  return ERR_PTR(-EINVAL);
   465          }
   466  
   467          qp_grp = kzalloc(sizeof(*qp_grp), GFP_KERNEL);
                                                  ^^^^^^^^^^
Sleeping allocation.

   468          if (!qp_grp) {
   469                  usnic_err("Unable to alloc qp_grp - Out of memory\n");
   470                  return NULL;
   471          }

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: IB/usnic: Add Cisco VIC low-level hardware driver

[Upinder Malhi (umalhi)]

Why not?  

We can switch to WARN_ON.  The kernel provides this macro - assert_spin_locked - which does asserts via BUG_ON that spin_is_locked, which makes me think that using BUG_ON in conjunction with spin_is_locked is legal.  We should use this macro instead of manually asserting, unless that it is not legal, in which case WARN_ON will do.

Upinder

On Dec 11, 2013, at 2:40 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:

> Hello Upinder Malhi,
> 
> The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware
> driver" from Sep 10, 2013, leads to the following Smatch
> warning:
> 	drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c:467
> 	usnic_ib_qp_grp_create()
> 	error: scheduling with locks held: 'spin_lock:lock'
> 
> drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c
>   449          BUG_ON(!spin_is_locked(&vf->lock));
>                        ^^^^^^^^^^^^^^^^^^^^^^^^
> Holding lock.
> Don't call BUG_ON(), use WARN_ON()?
> 
>   450  
>   451          err = usnic_vnic_res_spec_satisfied(&min_transport_spec[transport],
>   452                                                  res_spec);
>   453          if (err) {
>   454                  usnic_err("Spec does not meet miniumum req for transport %d\n",
>   455                                  transport);
>   456                  log_spec(res_spec);
>   457                  return ERR_PTR(err);
>   458          }
>   459  
>   460          port_num = usnic_transport_rsrv_port(transport, 0);
>   461          if (!port_num) {
>   462                  usnic_err("Unable to allocate port for %s\n",
>   463                                  netdev_name(ufdev->netdev));
>   464                  return ERR_PTR(-EINVAL);
>   465          }
>   466  
>   467          qp_grp = kzalloc(sizeof(*qp_grp), GFP_KERNEL);
>                                                  ^^^^^^^^^^
> Sleeping allocation.
> 
>   468          if (!qp_grp) {
>   469                  usnic_err("Unable to alloc qp_grp - Out of memory\n");
>   470                  return NULL;
>   471          }
> 
> regards,
> dan carpenter
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: IB/usnic: Add Cisco VIC low-level hardware driver

[Dan Carpenter]

On Thu, Dec 12, 2013 at 03:03:15AM +0000, Upinder Malhi (umalhi) wrote:
> Why not?  
> 
> We can switch to WARN_ON.  The kernel provides this macro -
> assert_spin_locked - which does asserts via BUG_ON that
> spin_is_locked, which makes me think that using BUG_ON in conjunction
> with spin_is_locked is legal.  We should use this macro instead of
> manually asserting, unless that it is not legal, in which case WARN_ON
> will do.

It's better to not halt the kernel.  That way you can do some debugging.

But the main thing is the scheduling with a spin lock held.  The other
I don't care about.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

re: IB/usnic: Add Cisco VIC low-level hardware driver

[Dan Carpenter]

Hello Upinder Malhi,

The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following Smatch
warning:

	drivers/infiniband/hw/usnic/usnic_uiom.c:560
	usnic_uiom_get_dev_list()
	error: scheduling with locks held: 'spin_lock:lock'

drivers/infiniband/hw/usnic/usnic_uiom.c
   553  struct device **usnic_uiom_get_dev_list(struct usnic_uiom_pd *pd)
   554  {
   555          struct usnic_uiom_dev *uiom_dev;
   556          struct device **devs;
   557          int i = 0;
   558  
   559          spin_lock(&pd->lock);
   560          devs = kzalloc(sizeof(*devs)*(pd->dev_cnt + 1), GFP_KERNEL);
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
You can't do blocking allocations with a spin_lock held.

   561          if (!devs) {
   562                  devs = ERR_PTR(-ENOMEM);
   563                  goto out;
   564          }

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

re: IB/usnic: Add Cisco VIC low-level hardware driver

[Dan Carpenter]

Hello Upinder Malhi,

The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following static checker
warning:
	drivers/infiniband/hw/usnic/usnic_uiom.c:47 usnic_uiom_alloc_pd()
	warn: passing zero to 'PTR_ERR'"

drivers/infiniband/hw/usnic/usnic_uiom.c
   469          pd->domain = domain = iommu_domain_alloc(&pci_bus_type);
                                      ^^^^^^^^^^^^^^^^^^^
This function returns NULL on error not error pointers.

   470          if (IS_ERR_OR_NULL(domain)) {
   471                  usnic_err("Failed to allocate IOMMU domain with err %ld\n",
   472                                  PTR_ERR(pd->domain));
   473                  kfree(pd);
   474                  return ERR_PTR(domain ? PTR_ERR(domain) : -ENOMEM);
   475          }

Similar harmless but crappy slop in:

vers/infiniband/hw/usnic/usnic_ib_main.c
   249          us_ibdev = (struct usnic_ib_dev *)ib_alloc_device(sizeof(*us_ibdev));
   250          if (IS_ERR_OR_NULL(us_ibdev)) {
                    ^^^^^^^^^^^^^^^^^^^^^^^^
   251                  usnic_err("Device %s context alloc failed\n",
   252                                  netdev_name(pci_get_drvdata(dev)));
   253                  return ERR_PTR(us_ibdev ? PTR_ERR(us_ibdev) : -EFAULT);
   254          }
   255  
   256          us_ibdev->ufdev = usnic_fwd_dev_alloc(dev);
   257          if (IS_ERR_OR_NULL(us_ibdev->ufdev)) {
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   258                  usnic_err("Failed to alloc ufdev for %s with err %ld\n",
   259                                  pci_name(dev), PTR_ERR(us_ibdev->ufdev));
   260                  goto err_dealloc;
   261          }

The general confusing about what return values are leads to a bug later
on:

vers/infiniband/hw/usnic/usnic_ib_main.c
   462          pf = usnic_ib_discover_pf(vf->vnic);
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This function returns ERR_PTRs.  Also it has a bug and can return freed
pointers.  Oops...  :(

   463          if (!pf) {
   464                  usnic_err("Failed to discover pf of vnic %s with err%d\n",
   465                                  pci_name(pdev), err);
   466                  goto out_clean_vnic;
   467          }
   468  
   469          vf->pf = pf;
   470          spin_lock_init(&vf->lock);
   471          mutex_lock(&pf->usdev_lock);

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: IB/usnic: Add Cisco VIC low-level hardware driver

[Upinder Malhi (umalhi)]

Hi Dan,
	Can I ask what static checker you are using for these?

See Inline for rest.
On Dec 11, 2013, at 2:38 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:

> Hello Upinder Malhi,
> 
> The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware
> driver" from Sep 10, 2013, leads to the following static checker
> warning:
> 	drivers/infiniband/hw/usnic/usnic_uiom.c:47 usnic_uiom_alloc_pd()
> 	warn: passing zero to 'PTR_ERR'"
> 
> drivers/infiniband/hw/usnic/usnic_uiom.c
>   469          pd->domain = domain = iommu_domain_alloc(&pci_bus_type);
>                                      ^^^^^^^^^^^^^^^^^^^
> This function returns NULL on error not error pointers.
> 
>   470          if (IS_ERR_OR_NULL(domain)) {
>   471                  usnic_err("Failed to allocate IOMMU domain with err %ld\n",
>   472                                  PTR_ERR(pd->domain));
>   473                  kfree(pd);
>   474                  return ERR_PTR(domain ? PTR_ERR(domain) : -ENOMEM);
>   475          }
> 
> Similar harmless but crappy slop in:
[UM] - Unless I'm missing something, I think this is OK and preferred in case iommu_domain_alloc in future starts returning ERR_PTRs.
> 
> vers/infiniband/hw/usnic/usnic_ib_main.c
>   249          us_ibdev = (struct usnic_ib_dev *)ib_alloc_device(sizeof(*us_ibdev));
>   250          if (IS_ERR_OR_NULL(us_ibdev)) {
>                    ^^^^^^^^^^^^^^^^^^^^^^^^
>   251                  usnic_err("Device %s context alloc failed\n",
>   252                                  netdev_name(pci_get_drvdata(dev)));
>   253                  return ERR_PTR(us_ibdev ? PTR_ERR(us_ibdev) : -EFAULT);
>   254          }
>   255  
>   256          us_ibdev->ufdev = usnic_fwd_dev_alloc(dev);
>   257          if (IS_ERR_OR_NULL(us_ibdev->ufdev)) {
>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   258                  usnic_err("Failed to alloc ufdev for %s with err %ld\n",
>   259                                  pci_name(dev), PTR_ERR(us_ibdev->ufdev));
>   260                  goto err_dealloc;
>   261          }
> 
> The general confusing about what return values are leads to a bug later
> on:
> 
> vers/infiniband/hw/usnic/usnic_ib_main.c
>   462          pf = usnic_ib_discover_pf(vf->vnic);
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This function returns ERR_PTRs.  Also it has a bug and can return freed
> pointers.  Oops...  :(
[UM] - Yes, that is oops indeed.  Will fix.
> 
>   463          if (!pf) {
>   464                  usnic_err("Failed to discover pf of vnic %s with err%d\n",
>   465                                  pci_name(pdev), err);
>   466                  goto out_clean_vnic;
>   467          }
>   468  
>   469          vf->pf = pf;
>   470          spin_lock_init(&vf->lock);
>   471          mutex_lock(&pf->usdev_lock);
> 
> regards,
> dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: IB/usnic: Add Cisco VIC low-level hardware driver

[Dan Carpenter]

On Thu, Dec 12, 2013 at 01:59:00AM +0000, Upinder Malhi (umalhi) wrote:
> Hi Dan,
> 	Can I ask what static checker you are using for these?
> 

In this email, the released version of Smatch will only warn about the
actual bug and not the others.  It requires that you build the cross
function database though before you test the code.

~/smatch/smatch_scripts/build_kernel_data.sh

> > drivers/infiniband/hw/usnic/usnic_uiom.c
> >   469          pd->domain = domain = iommu_domain_alloc(&pci_bus_type);
> >                                      ^^^^^^^^^^^^^^^^^^^
> > This function returns NULL on error not error pointers.
> > 
> >   470          if (IS_ERR_OR_NULL(domain)) {
> >   471                  usnic_err("Failed to allocate IOMMU domain with err %ld\n",
> >   472                                  PTR_ERR(pd->domain));
> >   473                  kfree(pd);
> >   474                  return ERR_PTR(domain ? PTR_ERR(domain) : -ENOMEM);
> >   475          }
> > 
> > Similar harmless but crappy slop in:
> [UM] - Unless I'm missing something, I think this is OK and preferred
> in case iommu_domain_alloc in future starts returning ERR_PTRs.

It doesn't cause a bug, but it's unusual and considered sloppy.  APIs
should be clear about what error codes they return and they should
almost always return either ERR_PTRs *or* NULL but not *both*.  The
exceptions are when NULL is a valid return which means something.  For
example, maybe you are looking for a file and you return NULL if the
file is not found but ERR_PTR(-ENOMEM) if you have an error.

In linux if we decide that iommu_domain_alloc() should return ERR_PTRs
instead of NULL pointers, then it's up to the person who decides that
to change all the callers as well.

Also if you trust the lower levels to print their error messages
correctly then the error handling here could be much simpler.  kzalloc()
for example, has very extensive error messages.

	pd->domain = domain = iommu_domain_alloc(&pci_bus_type);
	if (!domain) {
		kfree(pd);
		return -ENOMEM;
	}

regards,
dan carpenter


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html