| Message ID | 20240130060517.19942-1-caoqq@fujitsu.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl/hdm: Enhance handling of invalid decoders | expand |
Quanquan Cao wrote: > Add the condition check "base + size < base", enhanced > handling of invalid decoders. This check ensures the > decoder's address range calculation won't overflow. > > Signed-off-by: Quanquan Cao <caoqq@fujitsu.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 7d97790b893d..b8978d1c7a24 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -816,7 +816,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, > > if (!committed) > size = 0; > - if (base == U64_MAX || size == U64_MAX) { > + if (base == U64_MAX || size == U64_MAX || base + size < base) { The U64_MAX checks were added for a device that returned all 0xffffffffffffffff on read. That happens to match the common expectation that a device or a bus in an error state starts returning that value to MMIO cycles. So this was less about validating that those values were sane and more about having a convenient point to detect that the device is returning the common indicator for MMIO failure. Otherwise a device that reports an overflow condition is definitely broken, but it will be caught by other address validation code. So, I do not think this is suitable otherwise it would open the door for many other device validation checks. I would change my mind if there was an example of this breakage in the wild causing real end user pain that is too late for the device hardware vendor to fix.
> Quanquan Cao wrote: >> Add the condition check "base + size < base", enhanced >> handling of invalid decoders. This check ensures the >> decoder's address range calculation won't overflow. >> >> Signed-off-by: Quanquan Cao <caoqq@fujitsu.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 7d97790b893d..b8978d1c7a24 100644 >> --- a/drivers/cxl/core/hdm.c >> +++ b/drivers/cxl/core/hdm.c >> @@ -816,7 +816,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, >> >> if (!committed) >> size = 0; >> - if (base == U64_MAX || size == U64_MAX) { >> + if (base == U64_MAX || size == U64_MAX || base + size < base) { > > The U64_MAX checks were added for a device that returned all > 0xffffffffffffffff on read. That happens to match the common expectation > that a device or a bus in an error state starts returning that value to > MMIO cycles. So this was less about validating that those values were > sane and more about having a convenient point to detect that the device > is returning the common indicator for MMIO failure. > > Otherwise a device that reports an overflow condition is definitely > broken, but it will be caught by other address validation code. So, I > do not think this is suitable otherwise it would open the door for many > other device validation checks. > > I would change my mind if there was an example of this breakage in the > wild causing real end user pain that is too late for the device hardware > vendor to fix. > Thanks, I got.
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 7d97790b893d..b8978d1c7a24 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -816,7 +816,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, if (!committed) size = 0; - if (base == U64_MAX || size == U64_MAX) { + if (base == U64_MAX || size == U64_MAX || base + size < base) { dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n", port->id, cxld->id); return -ENXIO;
Add the condition check "base + size < base", enhanced handling of invalid decoders. This check ensures the decoder's address range calculation won't overflow. Signed-off-by: Quanquan Cao <caoqq@fujitsu.com> --- drivers/cxl/core/hdm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)