[PATCH] cxl/region: Fix null pointer dereference due to pass through decoder commit

[PATCH] cxl/region: Fix null pointer dereference due to pass through decoder commit

[Jonathan Cameron]

Not all decoders have a commit callback.

The CXL specification allows a host bridge with a single root port to
have no explicit HDM decoders. Currently we assumes there are none.
As such we create a special pass through decoder instance without
a commit callback.

Prior to this patch, the commit callback was called unconditionally.
Thus a configuration with 1 Host Bridge, 1 Root Port, 1 switch with
multiple downstream ports below which there are multiple CXL type 3
devices results in a situation where committing the region causes
a null pointer dereference.

Reported-by: Bobo WL <lmw.bobo@gmail.com>
Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---

We could fix this with a stub function perhaps as an alternative?

 drivers/cxl/core/region.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 401148016978..c49d9a5f1091 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -174,7 +174,8 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
 		     iter = to_cxl_port(iter->dev.parent)) {
 			cxl_rr = cxl_rr_load(iter, cxlr);
 			cxld = cxl_rr->decoder;
-			rc = cxld->commit(cxld);
+			if (cxld->commit)
+				rc = cxld->commit(cxld);
 			if (rc)
 				break;
 		}
-- 
2.32.0

Re: [PATCH] cxl/region: Fix null pointer dereference due to pass through decoder commit

[Verma, Vishal L]

On Thu, 2022-08-18 at 17:42 +0100, Jonathan Cameron wrote:
> Not all decoders have a commit callback.
> 
> The CXL specification allows a host bridge with a single root port to
> have no explicit HDM decoders. Currently we assumes there are none.

s/assumes/assume/ or,
'the region driver' instead of 'we'?

> As such we create a special pass through decoder instance without
> a commit callback.
> 
> Prior to this patch, the commit callback was called unconditionally.
> Thus a configuration with 1 Host Bridge, 1 Root Port, 1 switch with
> multiple downstream ports below which there are multiple CXL type 3
> devices results in a situation where committing the region causes
> a null pointer dereference.
> 
> Reported-by: Bobo WL <lmw.bobo@gmail.com>
> Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> 
> We could fix this with a stub function perhaps as an alternative?
> 
>  drivers/cxl/core/region.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Other than that, looks good to me,

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 401148016978..c49d9a5f1091 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -174,7 +174,8 @@ static int cxl_region_decode_commit(struct
> cxl_region *cxlr)
>                      iter = to_cxl_port(iter->dev.parent)) {
>                         cxl_rr = cxl_rr_load(iter, cxlr);
>                         cxld = cxl_rr->decoder;
> -                       rc = cxld->commit(cxld);
> +                       if (cxld->commit)
> +                               rc = cxld->commit(cxld);
>                         if (rc)
>                                 break;
>                 }

RE: [PATCH] cxl/region: Fix null pointer dereference due to pass through decoder commit

[Dan Williams]

Jonathan Cameron wrote:
> Not all decoders have a commit callback.
> 
> The CXL specification allows a host bridge with a single root port to
> have no explicit HDM decoders. Currently we assumes there are none.
> As such we create a special pass through decoder instance without
> a commit callback.
> 
> Prior to this patch, the commit callback was called unconditionally.
> Thus a configuration with 1 Host Bridge, 1 Root Port, 1 switch with
> multiple downstream ports below which there are multiple CXL type 3
> devices results in a situation where committing the region causes
> a null pointer dereference.
> 
> Reported-by: Bobo WL <lmw.bobo@gmail.com>
> Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> 
> We could fix this with a stub function perhaps as an alternative?

How about keep the callback mandatory except for the passthrough switch
case?

Something like this (untested):

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 401148016978..b1137f6dc2d1 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -172,11 +172,17 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
                /* commit bottom up */
                for (iter = cxled_to_port(cxled); !is_cxl_root(iter);
                     iter = to_cxl_port(iter->dev.parent)) {
+                       struct cxl_switch_decoder *cxlsd = NULL;
+
                        cxl_rr = cxl_rr_load(iter, cxlr);
                        cxld = cxl_rr->decoder;
-                       rc = cxld->commit(cxld);
-                       if (rc)
-                               break;
+                       if (is_switch_decoder(&cxld->dev))
+                               cxlsd = to_cxl_switch_decoder(&cxld->dev);
+                       if (cxld->commit || cxlsd && cxlsd->nr_targets > 1) {
+                               rc = cxld->commit(cxld);
+                               if (rc)
+                                       break;
+                       }
                }
 
                if (rc) {

Re: [PATCH] cxl/region: Fix null pointer dereference due to pass through decoder commit

[Jonathan Cameron]

On Thu, 18 Aug 2022 17:37:22 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > Not all decoders have a commit callback.
> > 
> > The CXL specification allows a host bridge with a single root port to
> > have no explicit HDM decoders. Currently we assumes there are none.
> > As such we create a special pass through decoder instance without
> > a commit callback.
> > 
> > Prior to this patch, the commit callback was called unconditionally.
> > Thus a configuration with 1 Host Bridge, 1 Root Port, 1 switch with
> > multiple downstream ports below which there are multiple CXL type 3
> > devices results in a situation where committing the region causes
> > a null pointer dereference.
> > 
> > Reported-by: Bobo WL <lmw.bobo@gmail.com>
> > Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> > 
> > We could fix this with a stub function perhaps as an alternative?  
> 
> How about keep the callback mandatory except for the passthrough switch
> case?

Should work, but makes it messier if we do add support for HDM decoders
in the pass through case (which I'm 90% + sure we will have once actual
platforms appear - we regularly have PCIe host bridges that on a particular
platform have one RP and on other platforms expose multiple - it's just a
wiring question - I doubt we'll bother hiding the HDMs given the spec
allows for them to be there in this case and it adds complexity to the
design to hide them).

Keeping the cxlsd->nr_targets > 1 check in one place would be my preference
or adding a wrapper. e.g.
is_passthrough_decoder()
that we can change easily later.

Either way, focus now is getting fix in, can change how it's done
later.  So I'm fine with either fix.

Jonathan




> 
> Something like this (untested):
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 401148016978..b1137f6dc2d1 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -172,11 +172,17 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
>                 /* commit bottom up */
>                 for (iter = cxled_to_port(cxled); !is_cxl_root(iter);
>                      iter = to_cxl_port(iter->dev.parent)) {
> +                       struct cxl_switch_decoder *cxlsd = NULL;
> +
>                         cxl_rr = cxl_rr_load(iter, cxlr);
>                         cxld = cxl_rr->decoder;
> -                       rc = cxld->commit(cxld);
> -                       if (rc)
> -                               break;
> +                       if (is_switch_decoder(&cxld->dev))
> +                               cxlsd = to_cxl_switch_decoder(&cxld->dev);
> +                       if (cxld->commit || cxlsd && cxlsd->nr_targets > 1) {
> +                               rc = cxld->commit(cxld);
> +                               if (rc)
> +                                       break;
> +                       }
>                 }
>  
>                 if (rc) {
> 

Re: [PATCH] cxl/region: Fix null pointer dereference due to pass through decoder commit

[Jonathan Cameron]

On Fri, 19 Aug 2022 09:31:33 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Thu, 18 Aug 2022 17:37:22 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Jonathan Cameron wrote:  
> > > Not all decoders have a commit callback.
> > > 
> > > The CXL specification allows a host bridge with a single root port to
> > > have no explicit HDM decoders. Currently we assumes there are none.
> > > As such we create a special pass through decoder instance without
> > > a commit callback.
> > > 
> > > Prior to this patch, the commit callback was called unconditionally.
> > > Thus a configuration with 1 Host Bridge, 1 Root Port, 1 switch with
> > > multiple downstream ports below which there are multiple CXL type 3
> > > devices results in a situation where committing the region causes
> > > a null pointer dereference.
> > > 
> > > Reported-by: Bobo WL <lmw.bobo@gmail.com>
> > > Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---
> > > 
> > > We could fix this with a stub function perhaps as an alternative?    
> > 
> > How about keep the callback mandatory except for the passthrough switch
> > case?  
> 
> Should work, but makes it messier if we do add support for HDM decoders
> in the pass through case (which I'm 90% + sure we will have once actual
> platforms appear - we regularly have PCIe host bridges that on a particular
> platform have one RP and on other platforms expose multiple - it's just a
> wiring question - I doubt we'll bother hiding the HDMs given the spec
> allows for them to be there in this case and it adds complexity to the
> design to hide them).
> 
> Keeping the cxlsd->nr_targets > 1 check in one place would be my preference
> or adding a wrapper. e.g.
> is_passthrough_decoder()
> that we can change easily later.
> 
> Either way, focus now is getting fix in, can change how it's done
> later.  So I'm fine with either fix.
> 
> Jonathan
> 
> 

Hi Dan,

I forgot to test your suggestion. Now done and it works as expected.

One comment below.

Given this is now your code, do you want to send the patch or is it
easier if I do so?
 
> 
> 
> > 
> > Something like this (untested):
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 401148016978..b1137f6dc2d1 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -172,11 +172,17 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
> >                 /* commit bottom up */
> >                 for (iter = cxled_to_port(cxled); !is_cxl_root(iter);
> >                      iter = to_cxl_port(iter->dev.parent)) {
> > +                       struct cxl_switch_decoder *cxlsd = NULL;
> > +
> >                         cxl_rr = cxl_rr_load(iter, cxlr);
> >                         cxld = cxl_rr->decoder;
> > -                       rc = cxld->commit(cxld);
> > -                       if (rc)
> > -                               break;
> > +                       if (is_switch_decoder(&cxld->dev))
> > +                               cxlsd = to_cxl_switch_decoder(&cxld->dev);
> > +                       if (cxld->commit || cxlsd && cxlsd->nr_targets > 1) {

I'd add brackets around the (cxlsd && cxlsd->nr_targets > 1) 
just to stop forgetful people like me having to check the precedence ordering..
 
> > +                               rc = cxld->commit(cxld);
> > +                               if (rc)
> > +                                       break;
> > +                       }
> >                 }
> >  
> >                 if (rc) {
> >   
> 

Re: [PATCH] cxl/region: Fix null pointer dereference due to pass through decoder commit

[Dan Williams]

Jonathan Cameron wrote:
> On Fri, 19 Aug 2022 09:31:33 +0100
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> > On Thu, 18 Aug 2022 17:37:22 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> > 
> > > Jonathan Cameron wrote:  
> > > > Not all decoders have a commit callback.
> > > > 
> > > > The CXL specification allows a host bridge with a single root port to
> > > > have no explicit HDM decoders. Currently we assumes there are none.
> > > > As such we create a special pass through decoder instance without
> > > > a commit callback.
> > > > 
> > > > Prior to this patch, the commit callback was called unconditionally.
> > > > Thus a configuration with 1 Host Bridge, 1 Root Port, 1 switch with
> > > > multiple downstream ports below which there are multiple CXL type 3
> > > > devices results in a situation where committing the region causes
> > > > a null pointer dereference.
> > > > 
> > > > Reported-by: Bobo WL <lmw.bobo@gmail.com>
> > > > Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > ---
> > > > 
> > > > We could fix this with a stub function perhaps as an alternative?    
> > > 
> > > How about keep the callback mandatory except for the passthrough switch
> > > case?  
> > 
> > Should work, but makes it messier if we do add support for HDM decoders
> > in the pass through case (which I'm 90% + sure we will have once actual
> > platforms appear - we regularly have PCIe host bridges that on a particular
> > platform have one RP and on other platforms expose multiple - it's just a
> > wiring question - I doubt we'll bother hiding the HDMs given the spec
> > allows for them to be there in this case and it adds complexity to the
> > design to hide them).
> > 
> > Keeping the cxlsd->nr_targets > 1 check in one place would be my preference
> > or adding a wrapper. e.g.
> > is_passthrough_decoder()
> > that we can change easily later.
> > 
> > Either way, focus now is getting fix in, can change how it's done
> > later.  So I'm fine with either fix.
> > 
> > Jonathan
> > 
> > 
> 
> Hi Dan,
> 
> I forgot to test your suggestion. Now done and it works as expected.
> 
> One comment below.
> 
> Given this is now your code, do you want to send the patch or is it
> easier if I do so?

I will take your fix since it is easier to backport, and then follow-up
with a helper to make the expectations clearer for multi-target
decoders.

Re: [PATCH] cxl/region: Fix null pointer dereference due to pass through decoder commit

[Dan Williams]

Verma, Vishal L wrote:
> On Thu, 2022-08-18 at 17:42 +0100, Jonathan Cameron wrote:
> > Not all decoders have a commit callback.
> > 
> > The CXL specification allows a host bridge with a single root port to
> > have no explicit HDM decoders. Currently we assumes there are none.
> 
> s/assumes/assume/ or,
> 'the region driver' instead of 'we'?
> 
> > As such we create a special pass through decoder instance without
> > a commit callback.
> > 
> > Prior to this patch, the commit callback was called unconditionally.
> > Thus a configuration with 1 Host Bridge, 1 Root Port, 1 switch with
> > multiple downstream ports below which there are multiple CXL type 3
> > devices results in a situation where committing the region causes
> > a null pointer dereference.
> > 
> > Reported-by: Bobo WL <lmw.bobo@gmail.com>
> > Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> > 
> > We could fix this with a stub function perhaps as an alternative?
> > 
> >  drivers/cxl/core/region.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Other than that, looks good to me,
> 
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

Thanks, incorporated the changelog fixups.