[PATCH] cxl/port: Validate decoder nr_targets against supported ways

[PATCH] cxl/port: Validate decoder nr_targets against supported ways

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

nr_targets is the number of downstream ports in a decoder's target
list. It is limited to the supported interleave ways: 1, 2, 3, 4, 6,
8, 12, 16. The existing check only enforces nr_targets <=
CXL_DECODER_MAX_INTERLEAVE (16), allowing invalid values, like 5, 7,
etc, to pass through.

Add is_nr_targets_valid() to check for supported interleave ways using
ways_to_eiw(). Use it before allocating a decoder to avoid alloc/free
churn when nr_targets is already known to be invalid.

This was found by inspection and hardens the code.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/port.c | 23 ++++++++++-------------
 drivers/cxl/cxl.h       | 11 +++++++++++
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 8f36ff413f5d..7d62797d1202 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1773,17 +1773,6 @@ static int cxl_decoder_init(struct cxl_port *port, struct cxl_decoder *cxld)
 	return 0;
 }
 
-static int cxl_switch_decoder_init(struct cxl_port *port,
-				   struct cxl_switch_decoder *cxlsd,
-				   int nr_targets)
-{
-	if (nr_targets > CXL_DECODER_MAX_INTERLEAVE)
-		return -EINVAL;
-
-	cxlsd->nr_targets = nr_targets;
-	return cxl_decoder_init(port, &cxlsd->cxld);
-}
-
 /**
  * cxl_root_decoder_alloc - Allocate a root level decoder
  * @port: owning CXL root of this decoder
@@ -1805,13 +1794,17 @@ struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port,
 	if (!is_cxl_root(port))
 		return ERR_PTR(-EINVAL);
 
+	if (!is_nr_targets_valid(nr_targets))
+		return ERR_PTR(-EINVAL);
+
 	cxlrd = kzalloc(struct_size(cxlrd, cxlsd.target, nr_targets),
 			GFP_KERNEL);
 	if (!cxlrd)
 		return ERR_PTR(-ENOMEM);
 
 	cxlsd = &cxlrd->cxlsd;
-	rc = cxl_switch_decoder_init(port, cxlsd, nr_targets);
+	cxlsd->nr_targets = nr_targets;
+	rc = cxl_decoder_init(port, &cxlsd->cxld);
 	if (rc) {
 		kfree(cxlrd);
 		return ERR_PTR(rc);
@@ -1859,11 +1852,15 @@ struct cxl_switch_decoder *cxl_switch_decoder_alloc(struct cxl_port *port,
 	if (is_cxl_root(port) || is_cxl_endpoint(port))
 		return ERR_PTR(-EINVAL);
 
+	if (!is_nr_targets_valid(nr_targets))
+		return ERR_PTR(-EINVAL);
+
 	cxlsd = kzalloc(struct_size(cxlsd, target, nr_targets), GFP_KERNEL);
 	if (!cxlsd)
 		return ERR_PTR(-ENOMEM);
 
-	rc = cxl_switch_decoder_init(port, cxlsd, nr_targets);
+	cxlsd->nr_targets = nr_targets;
+	rc = cxl_decoder_init(port, &cxlsd->cxld);
 	if (rc) {
 		kfree(cxlsd);
 		return ERR_PTR(rc);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 4fe3df06f57a..02b526a16492 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -132,6 +132,17 @@ static inline int ways_to_eiw(unsigned int ways, u8 *eiw)
 	return 0;
 }
 
+/* nr_targets must match a supported number of interleave ways */
+static inline bool is_nr_targets_valid(int nr_targets)
+{
+	u8 unused_eiw;
+
+	if (nr_targets <= 0)
+		return false;
+
+	return ways_to_eiw(nr_targets, &unused_eiw) == 0;
+}
+
 /* RAS Registers CXL 2.0 8.2.5.9 CXL RAS Capability Structure */
 #define CXL_RAS_UNCORRECTABLE_STATUS_OFFSET 0x0
 #define   CXL_RAS_UNCORRECTABLE_STATUS_MASK (GENMASK(16, 14) | GENMASK(11, 0))

base-commit: d9412f08e25a5b66f9021739c090cc9b8f1089b1
-- 
2.37.3

Re: [PATCH] cxl/port: Validate decoder nr_targets against supported ways

[dan.j.williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> nr_targets is the number of downstream ports in a decoder's target
> list. It is limited to the supported interleave ways: 1, 2, 3, 4, 6,
> 8, 12, 16. The existing check only enforces nr_targets <=
> CXL_DECODER_MAX_INTERLEAVE (16), allowing invalid values, like 5, 7,
> etc, to pass through.
> 
> Add is_nr_targets_valid() to check for supported interleave ways using
> ways_to_eiw(). Use it before allocating a decoder to avoid alloc/free
> churn when nr_targets is already known to be invalid.
> 
> This was found by inspection and hardens the code.

Well, now seeing this and trying to answer the risk question in 1/ led
to me to reconsider whether this is a good idea and whether we have a
deeper confusion at play:

1/ "Hardening" needs to come with an actual risk claim. For
   cxl_switch_decoder_alloc() the values come from hardware or ACPI and if
   those are buggy all bets are off. ACPI is already protected by a
   eiw_to_ways() conversion prior to calling cxl_root_decoder_alloc(), and
   hardware is even more accurate than this
   approach because it considers interleave ways capability.

2/ nr_targets != ways. ways_to_eiw() is the wrong validity check for
   switch decoders, nr_targets is a topology limitation, not a
   interleave_ways limitation. As far as I can see nothing breaks if a
   decoder claims to support a target count that is not a valid
   interleave_ways number.

So it was useful to go through this to get that clarified, but the
takeaway from 1/ is "there is insufficient risk" and the takeaway from
2/ is "nothing breaks even if switch decoder target count is 'invalid'".
The valid interleave_ways is a separate region-only property.

Re: [PATCH] cxl/port: Validate decoder nr_targets against supported ways

[Alison Schofield]

On Wed, Aug 13, 2025 at 05:57:50PM -0700, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > nr_targets is the number of downstream ports in a decoder's target
> > list. It is limited to the supported interleave ways: 1, 2, 3, 4, 6,
> > 8, 12, 16. The existing check only enforces nr_targets <=
> > CXL_DECODER_MAX_INTERLEAVE (16), allowing invalid values, like 5, 7,
> > etc, to pass through.
> > 
> > Add is_nr_targets_valid() to check for supported interleave ways using
> > ways_to_eiw(). Use it before allocating a decoder to avoid alloc/free
> > churn when nr_targets is already known to be invalid.
> > 
> > This was found by inspection and hardens the code.
> 
> Well, now seeing this and trying to answer the risk question in 1/ led
> to me to reconsider whether this is a good idea and whether we have a
> deeper confusion at play:
> 
> 1/ "Hardening" needs to come with an actual risk claim. For
>    cxl_switch_decoder_alloc() the values come from hardware or ACPI and if
>    those are buggy all bets are off. ACPI is already protected by a
>    eiw_to_ways() conversion prior to calling cxl_root_decoder_alloc(), and
>    hardware is even more accurate than this
>    approach because it considers interleave ways capability.

If all callers comes thru ACPI then agree no need. When you voiced
concern over valid host bridge interleave ways, which would be the
nr_targets of the root decoder (a switch decoder) I imagined you
were thinking of a path to root decoder alloc outside of ACPI.
Guess not.

> 
> 2/ nr_targets != ways. ways_to_eiw() is the wrong validity check for
>    switch decoders, nr_targets is a topology limitation, not a
>    interleave_ways limitation. As far as I can see nothing breaks if a
>    decoder claims to support a target count that is not a valid
>    interleave_ways number.

nr_targets does not have to equal interleave ways for the ways_to_eiw()
to be a useful helper for validating nr_targets. nr_targets has to be one
of 1,2,3,4,6,8,12,16 and if it's not ways_to_eiw() fails. I saw it as
simple code reuse.

I explored using cxl/test, changing nr_targets at decoder init time,
and predictable passes if greater than needed, and fails when less.
But it's not a thorough simulation and not worthy of a more detailed
simulation.

Thanks for the review. It was a worthwhile exploration :)

> 
> So it was useful to go through this to get that clarified, but the
> takeaway from 1/ is "there is insufficient risk" and the takeaway from
> 2/ is "nothing breaks even if switch decoder target count is 'invalid'".
> The valid interleave_ways is a separate region-only property.

Re: [PATCH] cxl/port: Validate decoder nr_targets against supported ways

[dan.j.williams]

Alison Schofield wrote:
> On Wed, Aug 13, 2025 at 05:57:50PM -0700, Dan Williams wrote:
> > alison.schofield@ wrote:
> > > From: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > nr_targets is the number of downstream ports in a decoder's target
> > > list. It is limited to the supported interleave ways: 1, 2, 3, 4, 6,
> > > 8, 12, 16. The existing check only enforces nr_targets <=
> > > CXL_DECODER_MAX_INTERLEAVE (16), allowing invalid values, like 5, 7,
> > > etc, to pass through.
> > > 
> > > Add is_nr_targets_valid() to check for supported interleave ways using
> > > ways_to_eiw(). Use it before allocating a decoder to avoid alloc/free
> > > churn when nr_targets is already known to be invalid.
> > > 
> > > This was found by inspection and hardens the code.
> > 
> > Well, now seeing this and trying to answer the risk question in 1/ led
> > to me to reconsider whether this is a good idea and whether we have a
> > deeper confusion at play:
> > 
> > 1/ "Hardening" needs to come with an actual risk claim. For
> >    cxl_switch_decoder_alloc() the values come from hardware or ACPI and if
> >    those are buggy all bets are off. ACPI is already protected by a
> >    eiw_to_ways() conversion prior to calling cxl_root_decoder_alloc(), and
> >    hardware is even more accurate than this
> >    approach because it considers interleave ways capability.
> 
> If all callers comes thru ACPI then agree no need. When you voiced
> concern over valid host bridge interleave ways, which would be the
> nr_targets of the root decoder (a switch decoder) I imagined you
> were thinking of a path to root decoder alloc outside of ACPI.
> Guess not.

I was looking at the patch in isolation and wondering out loud if we
needed something, but now upon seeing the patch, not so sure.

> > 2/ nr_targets != ways. ways_to_eiw() is the wrong validity check for
> >    switch decoders, nr_targets is a topology limitation, not a
> >    interleave_ways limitation. As far as I can see nothing breaks if a
> >    decoder claims to support a target count that is not a valid
> >    interleave_ways number.
> 
> nr_targets does not have to equal interleave ways for the ways_to_eiw()
> to be a useful helper for validating nr_targets. nr_targets has to be one
> of 1,2,3,4,6,8,12,16 and if it's not ways_to_eiw() fails. I saw it as
> simple code reuse.

See "Target Count" in 8.2.4.20.1 CXL HDM Decoder Capability Register.
The only hardware valid values today for target count are 1, 2, 4, and
8. That is different than ACPI CFMWS which can target 1,2,3,4,6,8,12,16
because CFMWS has a tighter relationship between nr_targets and
interleave_ways. It wants to define regions that target host bridges and
non-host-bridge targets.

The next question is, what if a switch decoder put 5 in its target count
field? Would the driver need to fall over immediately? I think the
answer is no. It would simply be the case that a decoder would set ways
to 1, 2, or 4 and select from the 5 potential targets.

So, I led you astray to make this a wider check beyond the root decoder
special case, and even there the risk of it being a problem in practice.
is low.

> I explored using cxl/test, changing nr_targets at decoder init time,
> and predictable passes if greater than needed, and fails when less.
> But it's not a thorough simulation and not worthy of a more detailed
> simulation.
> 
> Thanks for the review. It was a worthwhile exploration :)

Thanks for the patience to work through it with me! I learned something
about being more careful about the nr_targets:ways relationship.

Re: [PATCH] cxl/port: Validate decoder nr_targets against supported ways

[Alison Schofield]

On Thu, Aug 14, 2025 at 10:36:19AM -0700, Dan Williams wrote:
> Alison Schofield wrote:
> > On Wed, Aug 13, 2025 at 05:57:50PM -0700, Dan Williams wrote:
> > > alison.schofield@ wrote:
> > > > From: Alison Schofield <alison.schofield@intel.com>
> > > > 
> > > > nr_targets is the number of downstream ports in a decoder's target
> > > > list. It is limited to the supported interleave ways: 1, 2, 3, 4, 6,
> > > > 8, 12, 16. The existing check only enforces nr_targets <=
> > > > CXL_DECODER_MAX_INTERLEAVE (16), allowing invalid values, like 5, 7,
> > > > etc, to pass through.
> > > > 
> > > > Add is_nr_targets_valid() to check for supported interleave ways using
> > > > ways_to_eiw(). Use it before allocating a decoder to avoid alloc/free
> > > > churn when nr_targets is already known to be invalid.
> > > > 
> > > > This was found by inspection and hardens the code.
> > > 
> > > Well, now seeing this and trying to answer the risk question in 1/ led
> > > to me to reconsider whether this is a good idea and whether we have a
> > > deeper confusion at play:
> > > 
> > > 1/ "Hardening" needs to come with an actual risk claim. For
> > >    cxl_switch_decoder_alloc() the values come from hardware or ACPI and if
> > >    those are buggy all bets are off. ACPI is already protected by a
> > >    eiw_to_ways() conversion prior to calling cxl_root_decoder_alloc(), and
> > >    hardware is even more accurate than this
> > >    approach because it considers interleave ways capability.
> > 
> > If all callers comes thru ACPI then agree no need. When you voiced
> > concern over valid host bridge interleave ways, which would be the
> > nr_targets of the root decoder (a switch decoder) I imagined you
> > were thinking of a path to root decoder alloc outside of ACPI.
> > Guess not.
> 
> I was looking at the patch in isolation and wondering out loud if we
> needed something, but now upon seeing the patch, not so sure.
> 
> > > 2/ nr_targets != ways. ways_to_eiw() is the wrong validity check for
> > >    switch decoders, nr_targets is a topology limitation, not a
> > >    interleave_ways limitation. As far as I can see nothing breaks if a
> > >    decoder claims to support a target count that is not a valid
> > >    interleave_ways number.
> > 
> > nr_targets does not have to equal interleave ways for the ways_to_eiw()
> > to be a useful helper for validating nr_targets. nr_targets has to be one
> > of 1,2,3,4,6,8,12,16 and if it's not ways_to_eiw() fails. I saw it as
> > simple code reuse.
> 
> See "Target Count" in 8.2.4.20.1 CXL HDM Decoder Capability Register.
> The only hardware valid values today for target count are 1, 2, 4, and
> 8. That is different than ACPI CFMWS which can target 1,2,3,4,6,8,12,16
> because CFMWS has a tighter relationship between nr_targets and
> interleave_ways. It wants to define regions that target host bridges and
> non-host-bridge targets.
> 
> The next question is, what if a switch decoder put 5 in its target count
> field? Would the driver need to fall over immediately? I think the
> answer is no. It would simply be the case that a decoder would set ways
> to 1, 2, or 4 and select from the 5 potential targets.
> 
> So, I led you astray to make this a wider check beyond the root decoder
> special case, and even there the risk of it being a problem in practice.
> is low.
> 
> > I explored using cxl/test, changing nr_targets at decoder init time,
> > and predictable passes if greater than needed, and fails when less.
> > But it's not a thorough simulation and not worthy of a more detailed
> > simulation.
> > 
> > Thanks for the review. It was a worthwhile exploration :)
> 
> Thanks for the patience to work through it with me! I learned something
> about being more careful about the nr_targets:ways relationship.

I'm back, but only to share what 'finally' settled it in my mind. All I
saw was that CXL_MAX_DECODER_INTERLEAVE was set to 16 which happened to
to match the max region interleave ways. Dan pointed out the spec max
of 8, which made me go hmmmm again.

I had grep blinders on:

grep reveals this:
	cxl.h:#define CXL_DECODER_MAX_INTERLEAVE 16

reading the code reveals the well-placed, yet overlooked comment:
/*
 * Current specification goes up to 8, double that seems a reasonable
 * software max for the foreseeable future
 */
#define CXL_DECODER_MAX_INTERLEAVE 16

Got it now.
Also, with these small numbers, and the fact that the decoders target
count attribute only gets 4 bits, I no longer have concern about
useless allocations or alloc/free churn.