Re: [cxl-cxl:pending 39/40] drivers/cxl/core/bus.c:501 devm_cxl_add_decoder() warn: variable dereferenced before check 'cxld' (see line 497)

[Dan Carpenter]

On Wed, Aug 25, 2021 at 10:12:32AM +0300, Dan Carpenter wrote:
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  494  int devm_cxl_add_decoder(struct device *host, struct cxl_decoder *cxld,
> 574d46ed53b527 drivers/cxl/core/bus.c Dan Williams 2021-08-24  495  			 int *target_map)
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  496  {
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24 @497  	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
>                                                                                                             ^^^^^^^^^^^^^^^^
> Dereference
> 
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  498  	struct device *dev;
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  499  	int rc = 0, i;
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  500  
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24 @501  	if (!cxld)
>                                                                             ^^^^^
> Checked too late.
> 
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  502  		return -EINVAL;
> 574d46ed53b527 drivers/cxl/core/bus.c Dan Williams 2021-08-24  503  
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  504  	if (IS_ERR(cxld))
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  505  		return PTR_ERR(cxld);
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  506  
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  507  	if (cxld->interleave_ways < 1) {
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  508  		rc = -EINVAL;
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  509  		goto err;
> 
> "dev" not initialized at this point.
> 
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  510  	}
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  511  
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  512  	device_lock(&port->dev);
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  513  	if (list_empty(&port->dports))
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  514  		rc = -EINVAL;
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  515  
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  516  	for (i = 0; rc == 0 && target_map && i < cxld->nr_targets; i++) {
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  517  		struct cxl_dport *dport = find_dport(port, target_map[i]);
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  518  
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  519  		if (!dport) {
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  520  			rc = -ENXIO;
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  521  			break;
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  522  		}
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  523  		dev_dbg(host, "%s: target: %d\n", dev_name(dport->dport), i);
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  524  		cxld->target[i] = dport;
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  525  	}
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  526  	device_unlock(&port->dev);
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  527  	if (rc)
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  528  		goto err;
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  529  
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  530  	dev = &cxld->dev;
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  531  	rc = dev_set_name(dev, "decoder%d.%d", port->id, cxld->id);
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  532  	if (rc)
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  533  		goto err;
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  534  
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  535  	rc = device_add(dev);
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  536  	if (rc)
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  537  		goto err;
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  538  
> b7ca54b6255144 drivers/cxl/core/bus.c Dan Williams 2021-08-24  539  	return devm_add_action_or_reset(host, unregister_cxl_dev, dev);
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09  540  err:
> 40ba17afdfabb0 drivers/cxl/core.c     Dan Williams 2021-06-09 @541  	put_device(dev);
> 
> Should be:
> 
> 	put_device(&cxld->dev);
> 
> But it feels like a layering violation to drop a reference that was
> aquired by the caller.

This code hit linux-next yesterday so I reviewed it in context.  The
put_device() should just be removed.  It leads to a use after free.

regards,
dan carpenter