[PATCH] cxl/hdm: Avoid NULL deref when component registers are missing

[PATCH] cxl/hdm: Avoid NULL deref when component registers are missing

[Dan Williams]

The cxl_port driver attempts to support endpoint devices that do not
advertise a component register block, but by inspection
devm_cxl_setup_hdm() passes a NULL @crb to helper functions that should
be skipped.

Return early and skip setting target_count since that is only relevant
for switch decoders, not endpoint decoders.

Fixes: 757f6448b100 ("cxl/hdm: Fix double allocation of @cxlhdm")
Tested-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/hdm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 5293fe13fce3..cc123996b1a4 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -123,7 +123,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
 	crb = ioremap(port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
 	if (!crb && info && info->mem_enabled) {
 		cxlhdm->decoder_count = info->ranges;
-		cxlhdm->target_count = info->ranges;
+		return cxlhdm;
 	} else if (!crb) {
 		dev_err(dev, "No component registers mapped\n");
 		return ERR_PTR(-ENXIO);

Re: [PATCH] cxl/hdm: Avoid NULL deref when component registers are missing

[Dave Jiang]

On 3/29/23 2:35 PM, Dan Williams wrote:
> The cxl_port driver attempts to support endpoint devices that do not
> advertise a component register block, but by inspection
> devm_cxl_setup_hdm() passes a NULL @crb to helper functions that should
> be skipped.
> 
> Return early and skip setting target_count since that is only relevant
> for switch decoders, not endpoint decoders.
> 
> Fixes: 757f6448b100 ("cxl/hdm: Fix double allocation of @cxlhdm")
> Tested-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/core/hdm.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 5293fe13fce3..cc123996b1a4 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -123,7 +123,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>   	crb = ioremap(port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
>   	if (!crb && info && info->mem_enabled) {
>   		cxlhdm->decoder_count = info->ranges;
> -		cxlhdm->target_count = info->ranges;
> +		return cxlhdm;
>   	} else if (!crb) {
>   		dev_err(dev, "No component registers mapped\n");
>   		return ERR_PTR(-ENXIO);
> 

Re: [PATCH] cxl/hdm: Avoid NULL deref when component registers are missing

[Jonathan Cameron]

On Wed, 29 Mar 2023 14:35:43 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The cxl_port driver attempts to support endpoint devices that do not
> advertise a component register block, but by inspection
> devm_cxl_setup_hdm() passes a NULL @crb to helper functions that should
> be skipped.
> 
> Return early and skip setting target_count since that is only relevant
> for switch decoders, not endpoint decoders.

This is a good observation. It would be nice to not read it for the
HDM decoder path either. Obviously we don't use it so that doesn't do
any harm, but to someone reading the code it looks like we care about the
value.  I'm not immediately sure how we'd establish at this layer that
the HDM decoder is a switch or HB one though..

> 
> Fixes: 757f6448b100 ("cxl/hdm: Fix double allocation of @cxlhdm")
> Tested-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Patch looks fine to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/hdm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 5293fe13fce3..cc123996b1a4 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -123,7 +123,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>  	crb = ioremap(port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
>  	if (!crb && info && info->mem_enabled) {
>  		cxlhdm->decoder_count = info->ranges;
> -		cxlhdm->target_count = info->ranges;
> +		return cxlhdm;
>  	} else if (!crb) {
>  		dev_err(dev, "No component registers mapped\n");
>  		return ERR_PTR(-ENXIO);
> 

Re: [PATCH] cxl/hdm: Avoid NULL deref when component registers are missing

[Dan Williams]

Jonathan Cameron wrote:
> On Wed, 29 Mar 2023 14:35:43 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > The cxl_port driver attempts to support endpoint devices that do not
> > advertise a component register block, but by inspection
> > devm_cxl_setup_hdm() passes a NULL @crb to helper functions that should
> > be skipped.
> > 
> > Return early and skip setting target_count since that is only relevant
> > for switch decoders, not endpoint decoders.
> 
> This is a good observation. It would be nice to not read it for the
> HDM decoder path either. Obviously we don't use it so that doesn't do
> any harm, but to someone reading the code it looks like we care about the
> value.  I'm not immediately sure how we'd establish at this layer that
> the HDM decoder is a switch or HB one though..

@info is NULL when this routine is called for non-endpoint decoders.

> > Fixes: 757f6448b100 ("cxl/hdm: Fix double allocation of @cxlhdm")
> > Tested-by: Dave Jiang <dave.jiang@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Patch looks fine to me.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH] cxl/hdm: Avoid NULL deref when component registers are missing

[Jonathan Cameron]

On Thu, 30 Mar 2023 11:19:42 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Wed, 29 Mar 2023 14:35:43 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >   
> > > The cxl_port driver attempts to support endpoint devices that do not
> > > advertise a component register block, but by inspection
> > > devm_cxl_setup_hdm() passes a NULL @crb to helper functions that should
> > > be skipped.
> > > 
> > > Return early and skip setting target_count since that is only relevant
> > > for switch decoders, not endpoint decoders.  
> > 
> > This is a good observation. It would be nice to not read it for the
> > HDM decoder path either. Obviously we don't use it so that doesn't do
> > any harm, but to someone reading the code it looks like we care about the
> > value.  I'm not immediately sure how we'd establish at this layer that
> > the HDM decoder is a switch or HB one though..  
> 
> @info is NULL when this routine is called for non-endpoint decoders.

Ah, so we could pass a flag into parse_hdm_decoder_caps() and not read
the value if it has no meaning for the particular decoder.

> 
> > > Fixes: 757f6448b100 ("cxl/hdm: Fix double allocation of @cxlhdm")
> > > Tested-by: Dave Jiang <dave.jiang@intel.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> > Patch looks fine to me.
> > 
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  

Re: [PATCH] cxl/hdm: Avoid NULL deref when component registers are missing

[Dan Williams]

Jonathan Cameron wrote:
> On Thu, 30 Mar 2023 11:19:42 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Jonathan Cameron wrote:
> > > On Wed, 29 Mar 2023 14:35:43 -0700
> > > Dan Williams <dan.j.williams@intel.com> wrote:
> > >   
> > > > The cxl_port driver attempts to support endpoint devices that do not
> > > > advertise a component register block, but by inspection
> > > > devm_cxl_setup_hdm() passes a NULL @crb to helper functions that should
> > > > be skipped.
> > > > 
> > > > Return early and skip setting target_count since that is only relevant
> > > > for switch decoders, not endpoint decoders.  
> > > 
> > > This is a good observation. It would be nice to not read it for the
> > > HDM decoder path either. Obviously we don't use it so that doesn't do
> > > any harm, but to someone reading the code it looks like we care about the
> > > value.  I'm not immediately sure how we'd establish at this layer that
> > > the HDM decoder is a switch or HB one though..  
> > 
> > @info is NULL when this routine is called for non-endpoint decoders.
> 
> Ah, so we could pass a flag into parse_hdm_decoder_caps() and not read
> the value if it has no meaning for the particular decoder.

How about kerneldoc on 'struct cxl_hdm' clarifying @target_count and
other fields, because I don't see the benefit of logic to skip parsing
that field. The overhead of an MMIO cycle to read the capability
register has already been spent.

Re: [PATCH] cxl/hdm: Avoid NULL deref when component registers are missing

[Jonathan Cameron]

On Thu, 30 Mar 2023 11:49:26 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Thu, 30 Mar 2023 11:19:42 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >   
> > > Jonathan Cameron wrote:  
> > > > On Wed, 29 Mar 2023 14:35:43 -0700
> > > > Dan Williams <dan.j.williams@intel.com> wrote:
> > > >     
> > > > > The cxl_port driver attempts to support endpoint devices that do not
> > > > > advertise a component register block, but by inspection
> > > > > devm_cxl_setup_hdm() passes a NULL @crb to helper functions that should
> > > > > be skipped.
> > > > > 
> > > > > Return early and skip setting target_count since that is only relevant
> > > > > for switch decoders, not endpoint decoders.    
> > > > 
> > > > This is a good observation. It would be nice to not read it for the
> > > > HDM decoder path either. Obviously we don't use it so that doesn't do
> > > > any harm, but to someone reading the code it looks like we care about the
> > > > value.  I'm not immediately sure how we'd establish at this layer that
> > > > the HDM decoder is a switch or HB one though..    
> > > 
> > > @info is NULL when this routine is called for non-endpoint decoders.  
> > 
> > Ah, so we could pass a flag into parse_hdm_decoder_caps() and not read
> > the value if it has no meaning for the particular decoder.  
> 
> How about kerneldoc on 'struct cxl_hdm' clarifying @target_count and
> other fields, because I don't see the benefit of logic to skip parsing
> that field. The overhead of an MMIO cycle to read the capability
> register has already been spent.
> 

Ok. I guess it's harmless even it if gains meaning in some later spec
version as we don't use it for anything.

Jonathan