| Message ID | 7f7030bb14ad7c8e0e051319cf473ab3197da5be.1676043318.git.lukas@wunner.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Collection of DOE material | expand |
Lukas Wunner wrote: > cxl_cdat_get_length() only checks whether the DOE response size is > sufficient for the Table Access response header (1 dword), but not the > succeeding CDAT header (1 dword length plus other fields). > > It thus returns whatever uninitialized memory happens to be on the stack > if a truncated DOE response with only 1 dword was received. Fix it. > > Fixes: c97006046c79 ("cxl/port: Read CDAT table") > Reported-by: Ming Li <ming4.li@intel.com> > 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 | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index d3cf1d9d67d4..11a85b3a9a0b 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -528,7 +528,7 @@ static int cxl_cdat_get_length(struct device *dev, > return rc; > } > wait_for_completion(&t.c); > - if (t.task.rv < sizeof(u32)) > + if (t.task.rv < 2 * sizeof(u32)) > return -EIO; Looks good, I wonder since this is standard for all data objects whether the check should be pushed into the core? For now this is easier to backport, but a follow-on could push it down a level. Reviewed-by: Dan Williams <dan.j.williams@intel.com>
On Fri, Feb 10, 2023 at 04:40:15PM -0800, Dan Williams wrote: > Lukas Wunner wrote: > > cxl_cdat_get_length() only checks whether the DOE response size is > > sufficient for the Table Access response header (1 dword), but not the > > succeeding CDAT header (1 dword length plus other fields). > > > > It thus returns whatever uninitialized memory happens to be on the stack > > if a truncated DOE response with only 1 dword was received. Fix it. [...] > > --- a/drivers/cxl/core/pci.c > > +++ b/drivers/cxl/core/pci.c > > @@ -528,7 +528,7 @@ static int cxl_cdat_get_length(struct device *dev, > > return rc; > > } > > wait_for_completion(&t.c); > > - if (t.task.rv < sizeof(u32)) > > + if (t.task.rv < 2 * sizeof(u32)) > > return -EIO; > > Looks good, I wonder since this is standard for all data objects whether > the check should be pushed into the core? No, "t.task.rv" contains the payload length in bytes of the response received via DOE. It doesn't include the DOE header. I think it is legal to receive an empty response via DOE, so I cannot push a length check down into the core. In this case, the payload contains one dword for the Table Access Response header (CXL r3.0 sec 8.1.11.1), followed by 3 dwords for the CDAT header: https://uefi.org/sites/default/files/resources/Coherent%20Device%20Attribute%20Table_1.01.pdf The CDAT header contains the length of the entire CDAT in the first dword, hence the above-quoted check only verifies that at least two dwords were received. It's harmless if the remainder of the CDAT header is truncated. Thanks, Lukas
On Fri, 10 Feb 2023 21:25:02 +0100 Lukas Wunner <lukas@wunner.de> wrote: > cxl_cdat_get_length() only checks whether the DOE response size is > sufficient for the Table Access response header (1 dword), but not the > succeeding CDAT header (1 dword length plus other fields). > > It thus returns whatever uninitialized memory happens to be on the stack > if a truncated DOE response with only 1 dword was received. Fix it. > > Fixes: c97006046c79 ("cxl/port: Read CDAT table") > Reported-by: Ming Li <ming4.li@intel.com> > Tested-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: stable@vger.kernel.org # v6.0+ Oops + thanks for fixing. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > Changes v2 -> v3: > * Newly added patch in v3 > > drivers/cxl/core/pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index d3cf1d9d67d4..11a85b3a9a0b 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -528,7 +528,7 @@ static int cxl_cdat_get_length(struct device *dev, > return rc; > } > wait_for_completion(&t.c); > - if (t.task.rv < sizeof(u32)) > + if (t.task.rv < 2 * sizeof(u32)) > return -EIO; > > *length = le32_to_cpu(t.response_pl[1]);
On 2/11/2023 4:25 AM, Lukas Wunner wrote: > cxl_cdat_get_length() only checks whether the DOE response size is > sufficient for the Table Access response header (1 dword), but not the > succeeding CDAT header (1 dword length plus other fields). > > It thus returns whatever uninitialized memory happens to be on the stack > if a truncated DOE response with only 1 dword was received. Fix it. > > Fixes: c97006046c79 ("cxl/port: Read CDAT table") > Reported-by: Ming Li <ming4.li@intel.com> > Tested-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> Reviewed-by: Ming Li <ming4.li@intel.com>
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index d3cf1d9d67d4..11a85b3a9a0b 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -528,7 +528,7 @@ static int cxl_cdat_get_length(struct device *dev, return rc; } wait_for_completion(&t.c); - if (t.task.rv < sizeof(u32)) + if (t.task.rv < 2 * sizeof(u32)) return -EIO; *length = le32_to_cpu(t.response_pl[1]);
cxl_cdat_get_length() only checks whether the DOE response size is sufficient for the Table Access response header (1 dword), but not the succeeding CDAT header (1 dword length plus other fields). It thus returns whatever uninitialized memory happens to be on the stack if a truncated DOE response with only 1 dword was received. Fix it. Fixes: c97006046c79 ("cxl/port: Read CDAT table") Reported-by: Ming Li <ming4.li@intel.com> 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)