| Message ID | 4834ceab1c3e00d3ec957e6c8beb13ddaa9877a2.1676043318.git.lukas@wunner.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Collection of DOE material | expand |
Lukas Wunner wrote: > If the length in the CDAT header is larger than the concatenation of the > header and all table entries, then the CDAT exposed to user space > contains trailing null bytes. > > Not every consumer may be able to handle that. Per Postel's robustness > principle, "be liberal in what you accept" and silently reduce the > cached length to avoid exposing those null bytes. > > Fixes: c97006046c79 ("cxl/port: Read CDAT table") > Tested-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: stable@vger.kernel.org # v6.0+ > --- > Changes v2 -> v3: > * Newly added patch in v3 > > drivers/cxl/core/pci.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index a3fb6bd68d17..c37c41d7acb6 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -582,6 +582,9 @@ static int cxl_cdat_read_table(struct device *dev, > } > } while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY); > > + /* Length in CDAT header may exceed concatenation of CDAT entries */ > + cdat->length -= length; > + Looks good, Reviewed-by: Dan Williams <dan.j.williams@intel.com>
On Fri, 10 Feb 2023 21:25:04 +0100 Lukas Wunner <lukas@wunner.de> wrote: > If the length in the CDAT header is larger than the concatenation of the > header and all table entries, then the CDAT exposed to user space > contains trailing null bytes. > > Not every consumer may be able to handle that. Per Postel's robustness > principle, "be liberal in what you accept" and silently reduce the > cached length to avoid exposing those null bytes. > > Fixes: c97006046c79 ("cxl/port: Read CDAT table") > Tested-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: stable@vger.kernel.org # v6.0+ Fair enough. I'd argue that we are papering over broken hardware if we hit these conditions, so given we aren't aware of any (I hope) not sure this is stable material. Argument in favor of stable being that if we do get broken hardware we don't want an ABI change when we paper over the garbage... hmm. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > Changes v2 -> v3: > * Newly added patch in v3 > > drivers/cxl/core/pci.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index a3fb6bd68d17..c37c41d7acb6 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -582,6 +582,9 @@ static int cxl_cdat_read_table(struct device *dev, > } > } while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY); > > + /* Length in CDAT header may exceed concatenation of CDAT entries */ > + cdat->length -= length; > + > return 0; > } >
On Tue, Feb 14, 2023 at 11:33:11AM +0000, Jonathan Cameron wrote: > On Fri, 10 Feb 2023 21:25:04 +0100 Lukas Wunner <lukas@wunner.de> wrote: > > If the length in the CDAT header is larger than the concatenation of the > > header and all table entries, then the CDAT exposed to user space > > contains trailing null bytes. > > > > Not every consumer may be able to handle that. Per Postel's robustness > > principle, "be liberal in what you accept" and silently reduce the > > cached length to avoid exposing those null bytes. [...] > Fair enough. I'd argue that we are papering over broken hardware if > we hit these conditions, so given we aren't aware of any (I hope) > not sure this is stable material. Argument in favor of stable being > that if we do get broken hardware we don't want an ABI change when > we paper over the garbage... hmm. Type 0 is assigned for DSMAS structures. So user space might believe there's an additional DSMAS in the CDAT. It *could* detect that the length is bogus (it is 0 but should be 24), but what if it doesn't check that? It seems way too dangerous to leave this loophole open, hence the stable designation. Thanks, Lukas > > --- a/drivers/cxl/core/pci.c > > +++ b/drivers/cxl/core/pci.c > > @@ -582,6 +582,9 @@ static int cxl_cdat_read_table(struct device *dev, > > } > > } while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY); > > > > + /* Length in CDAT header may exceed concatenation of CDAT entries */ > > + cdat->length -= length; > > + > > return 0; > > } > >
On Thu, 16 Feb 2023 11:26:16 +0100 Lukas Wunner <lukas@wunner.de> wrote: > On Tue, Feb 14, 2023 at 11:33:11AM +0000, Jonathan Cameron wrote: > > On Fri, 10 Feb 2023 21:25:04 +0100 Lukas Wunner <lukas@wunner.de> wrote: > > > If the length in the CDAT header is larger than the concatenation of the > > > header and all table entries, then the CDAT exposed to user space > > > contains trailing null bytes. > > > > > > Not every consumer may be able to handle that. Per Postel's robustness > > > principle, "be liberal in what you accept" and silently reduce the > > > cached length to avoid exposing those null bytes. > [...] > > Fair enough. I'd argue that we are papering over broken hardware if > > we hit these conditions, so given we aren't aware of any (I hope) > > not sure this is stable material. Argument in favor of stable being > > that if we do get broken hardware we don't want an ABI change when > > we paper over the garbage... hmm. > > Type 0 is assigned for DSMAS structures. So user space might believe > there's an additional DSMAS in the CDAT. It *could* detect that the > length is bogus (it is 0 but should be 24), but what if it doesn't > check that? It seems way too dangerous to leave this loophole open, > hence the stable designation. Ok > > Thanks, > > Lukas > > > > --- a/drivers/cxl/core/pci.c > > > +++ b/drivers/cxl/core/pci.c > > > @@ -582,6 +582,9 @@ static int cxl_cdat_read_table(struct device *dev, > > > } > > > } while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY); > > > > > > + /* Length in CDAT header may exceed concatenation of CDAT entries */ > > > + cdat->length -= length; > > > + > > > return 0; > > > } > > >
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index a3fb6bd68d17..c37c41d7acb6 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -582,6 +582,9 @@ static int cxl_cdat_read_table(struct device *dev, } } while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY); + /* Length in CDAT header may exceed concatenation of CDAT entries */ + cdat->length -= length; + return 0; }
If the length in the CDAT header is larger than the concatenation of the header and all table entries, then the CDAT exposed to user space contains trailing null bytes. Not every consumer may be able to handle that. Per Postel's robustness principle, "be liberal in what you accept" and silently reduce the cached length to avoid exposing those null bytes. Fixes: c97006046c79 ("cxl/port: Read CDAT table") Tested-by: Ira Weiny <ira.weiny@intel.com> Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: stable@vger.kernel.org # v6.0+ --- Changes v2 -> v3: * Newly added patch in v3 drivers/cxl/core/pci.c | 3 +++ 1 file changed, 3 insertions(+)