linux-kernel - Re: [PATCH v1 1/1] cxl/hdm: Verify HDM decoder capabilities after parsing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z8IBuE714QNscgfJ@aschofie-mobl2.lan>
Date: Fri, 28 Feb 2025 10:34:32 -0800
From: Alison Schofield <alison.schofield@...el.com>
To: Li Ming <ming.li@...omail.com>
Cc: dave@...olabs.net, jonathan.cameron@...wei.com, dave.jiang@...el.com,
	vishal.l.verma@...el.com, ira.weiny@...el.com,
	dan.j.williams@...el.com, linux-cxl@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] cxl/hdm: Verify HDM decoder capabilities after
 parsing

On Fri, Feb 28, 2025 at 10:47:12AM +0800, Li Ming wrote:
> On 2/28/2025 5:47 AM, Alison Schofield wrote:
> > On Thu, Feb 27, 2025 at 06:32:51PM +0800, Li Ming wrote:
> >> devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM
> >> decoder capability, But according to the implementation of
> >> cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0.
> > How does a check against the spec maximums benefit this driver? Is there
> > a bad path we avoid by checking and quitting at this point.
> 
> 
> My understanding is that no a bad path on driver side if the decoder_count is greater than the maximum number spec defines.
> 
> Driver just allocates cxl decoders on the port based on the value of decoder_count. But I am not sure if hardware will have other potential problems when it didn't follow the spec.

I had the general thought that the driver is not responsible for
compliance checking the device, unless it affects function. Excessive
decoder_count's sound like they cause needless allocations, so let's
stop doing that - as best we can. 

Is it sufficient to clamp at the spec defined max values and continue
with only a dev_warn_once or even a dev_info?
ie. for a device: decoder_count = min(decoder_count, EP_DECODER_MAX);

That'll avoid failing a device that previously snuck by with an
excessive decoder_count and protect against excessive allocations
in the driver.

> 
> 
> >
> > Might this catch silly decoder counts that the driver previously
> > ignored?
> >
> >> Per CXL specification, the values ranges of decoder_count and
> >> target_count are limited. Adding a checking for the values of them
> >> in case hardware initialized them with wrong values.
> > Similar question - is this catching something sooner, rather than
> > later?
> 
> 
> Yes, the check is at the beginning of HDM setup during port probing, if value is wrong, will break HDM setup.
> 
> I'm not sure if I fully understand your question, please correct me if I misunderstand it. thanks.

I understand now. This one is different that decoder_count because 
it was heading to failure anyway and seems good to fail sooner.

> 
> 
> >
> >> Signed-off-by: Li Ming <ming.li@...omail.com>
> >> ---
> >> base-commit: 22eea823f69ae39dc060c4027e8d1470803d2e49 cxl/next
> >> ---
> >>  drivers/cxl/core/hdm.c | 31 ++++++++++++++++++++++++++++++-
> >>  1 file changed, 30 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> >> index 70cae4ebf8a4..a98191867c22 100644
> >> --- a/drivers/cxl/core/hdm.c
> >> +++ b/drivers/cxl/core/hdm.c
> >> @@ -138,6 +138,34 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> >>  	return true;
> >>  }
> >>  
> >> +static int cxlhdm_decoder_caps_verify(struct cxl_hdm *cxlhdm)
> >> +{
> >> +	/*
> >> +	 * CXL r3.2 section 8.2.4.20.1
> >> +	 * CXL devices shall not advertise more than 10 decoders,
> >> +	 * CXL switches and HBs may advertise up to 32 decoders.
> >> +	 */
> >> +	if (is_cxl_endpoint(cxlhdm->port) && cxlhdm->decoder_count > 10)
> >> +		return -EINVAL;
> >> +	else if (cxlhdm->decoder_count > 32)
> >> +		return -EINVAL;
> >> +
> >> +	/*
> >> +	 * CXL r3.2 section 8.2.4.20.1
> >> +	 * target count is applicable only to CXL upstream port and HB.
> >> +	 * The number of target ports each decoder supports should be
> >> +	 * one of the numbers 1, 2, 4 or 8.
> >> +	 */
> >> +	if (!is_cxl_endpoint(cxlhdm->port) &&
> >> +	    cxlhdm->target_count != 1 &&
> >> +	    cxlhdm->target_count != 2 &&
> >> +	    cxlhdm->target_count != 4 &&
> >> +	    cxlhdm->target_count != 8)
> >> +		return -EINVAL;
> > Maybe instead of manual bitwise checks try
> > 	(!is_power_of_2(cxlhdm->target_count) || cxlhdm->target_count > 8))
> 
> 
> Yes, It is clearer, thanks for that.
> 
> 
> >
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  /**
> >>   * devm_cxl_setup_hdm - map HDM decoder component registers
> >>   * @port: cxl_port to map
> >> @@ -182,7 +210,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
> >>  	}
> >>  
> >>  	parse_hdm_decoder_caps(cxlhdm);
> >> -	if (cxlhdm->decoder_count == 0) {
> >> +	rc = cxlhdm_decoder_caps_verify(cxlhdm);
> >> +	if (rc) {
> >>  		dev_err(dev, "Spec violation. Caps invalid\n");
> > Can you move the dev_err to the verify function and include the
> > specific invalid capability.
> >
> >
> > --Alison
> 
> Sure, will do that, thanks.
> 
> 
> Ming
> 
>