* [PATCH v3 1/2] cxl/acpi: Fix UAF in the error path
@ 2023-07-14 9:31 Breno Leitao
2023-07-14 9:31 ` [PATCH v3 2/2] cxl/acpi: Do not swallow errors Breno Leitao
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Breno Leitao @ 2023-07-14 9:31 UTC (permalink / raw)
To: alison.schofield, vishal.l.verma, ira.weiny, bwidawsk,
dan.j.williams
Cc: linux-cxl, dave.jiang
KASAN and KFENCE detected an user-after-free in the CXL driver. This
happens in the cxl_decoder_add() fail path. KASAN prints the following
error:
BUG: KASAN: slab-use-after-free in cxl_parse_cfmws (drivers/cxl/acpi.c:299)
This is happening in cxl_parse_cfmws(), where put_device() is called,
releasing cxld, which is accessed later.
Just use the local variables in the dev_err() instead of pointing to the
released memory.
Fixes: e50fe01e1f2a ("cxl/core: Drop ->platform_res attribute for root decoders")
Signed-off-by: Breno Leitao <leitao@debian.org>
---
v1 -> v2
* Return the error (rc) instead of swalling it
v2 -> v3
* Split the change in two patches
* Fix the reference instead of the order
drivers/cxl/acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 658e6b84a769..642983da01cb 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -297,7 +297,7 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
rc = cxl_decoder_autoremove(dev, cxld);
if (rc) {
dev_err(dev, "Failed to add decode range [%#llx - %#llx]\n",
- cxld->hpa_range.start, cxld->hpa_range.end);
+ res->start, res->end);
return 0;
}
dev_dbg(dev, "add: %s node: %d range [%#llx - %#llx]\n",
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v3 2/2] cxl/acpi: Do not swallow errors
2023-07-14 9:31 [PATCH v3 1/2] cxl/acpi: Fix UAF in the error path Breno Leitao
@ 2023-07-14 9:31 ` Breno Leitao
2023-07-14 20:50 ` Alison Schofield
2023-07-14 22:33 ` Verma, Vishal L
2023-07-14 20:50 ` [PATCH v3 1/2] cxl/acpi: Fix UAF in the error path Alison Schofield
2023-07-15 3:16 ` Dan Williams
2 siblings, 2 replies; 6+ messages in thread
From: Breno Leitao @ 2023-07-14 9:31 UTC (permalink / raw)
To: alison.schofield, vishal.l.verma, ira.weiny, bwidawsk,
dan.j.williams
Cc: linux-cxl, dave.jiang
Driver initialization is returning success (return 0) even if the
initialization (cxl_decoder_add() or acpi_table_parse_cedt()) failed,
and the hardware was already released (put_device()).
Return the error instead of swallowing it.
Fixes: f4ce1f766f1e ("cxl/acpi: Convert CFMWS parsing to ACPI sub-table helpers")
Signed-off-by: Breno Leitao <leitao@debian.org>
---
drivers/cxl/acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 642983da01cb..815b43859c16 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -298,7 +298,7 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
if (rc) {
dev_err(dev, "Failed to add decode range [%#llx - %#llx]\n",
res->start, res->end);
- return 0;
+ return rc;
}
dev_dbg(dev, "add: %s node: %d range [%#llx - %#llx]\n",
dev_name(&cxld->dev),
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3 2/2] cxl/acpi: Do not swallow errors
2023-07-14 9:31 ` [PATCH v3 2/2] cxl/acpi: Do not swallow errors Breno Leitao
@ 2023-07-14 20:50 ` Alison Schofield
2023-07-14 22:33 ` Verma, Vishal L
1 sibling, 0 replies; 6+ messages in thread
From: Alison Schofield @ 2023-07-14 20:50 UTC (permalink / raw)
To: Breno Leitao
Cc: vishal.l.verma, ira.weiny, bwidawsk, dan.j.williams, linux-cxl,
dave.jiang
On Fri, Jul 14, 2023 at 02:31:46AM -0700, Breno Leitao wrote:
> Driver initialization is returning success (return 0) even if the
> initialization (cxl_decoder_add() or acpi_table_parse_cedt()) failed,
> and the hardware was already released (put_device()).
>
> Return the error instead of swallowing it.
>
> Fixes: f4ce1f766f1e ("cxl/acpi: Convert CFMWS parsing to ACPI sub-table helpers")
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> ---
> drivers/cxl/acpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 642983da01cb..815b43859c16 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -298,7 +298,7 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
> if (rc) {
> dev_err(dev, "Failed to add decode range [%#llx - %#llx]\n",
> res->start, res->end);
> - return 0;
> + return rc;
> }
> dev_dbg(dev, "add: %s node: %d range [%#llx - %#llx]\n",
> dev_name(&cxld->dev),
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3 2/2] cxl/acpi: Do not swallow errors
2023-07-14 9:31 ` [PATCH v3 2/2] cxl/acpi: Do not swallow errors Breno Leitao
2023-07-14 20:50 ` Alison Schofield
@ 2023-07-14 22:33 ` Verma, Vishal L
1 sibling, 0 replies; 6+ messages in thread
From: Verma, Vishal L @ 2023-07-14 22:33 UTC (permalink / raw)
To: bwidawsk@kernel.org, Schofield, Alison, Williams, Dan J,
leitao@debian.org, Weiny, Ira
Cc: Jiang, Dave, linux-cxl@vger.kernel.org
On Fri, 2023-07-14 at 02:31 -0700, Breno Leitao wrote:
> Driver initialization is returning success (return 0) even if the
> initialization (cxl_decoder_add() or acpi_table_parse_cedt()) failed,
> and the hardware was already released (put_device()).
>
> Return the error instead of swallowing it.
>
> Fixes: f4ce1f766f1e ("cxl/acpi: Convert CFMWS parsing to ACPI sub-table helpers")
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
FYI there should not be a blank line between the Fixes tag and other
tags (Patch 1 has it right). This can break git's view of the trailers.
I fixed this up while applying and while at it did a bit of commit
message massaging for both patches.
> ---
> drivers/cxl/acpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 642983da01cb..815b43859c16 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -298,7 +298,7 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
> if (rc) {
> dev_err(dev, "Failed to add decode range [%#llx - %#llx]\n",
> res->start, res->end);
> - return 0;
> + return rc;
> }
> dev_dbg(dev, "add: %s node: %d range [%#llx - %#llx]\n",
> dev_name(&cxld->dev),
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] cxl/acpi: Fix UAF in the error path
2023-07-14 9:31 [PATCH v3 1/2] cxl/acpi: Fix UAF in the error path Breno Leitao
2023-07-14 9:31 ` [PATCH v3 2/2] cxl/acpi: Do not swallow errors Breno Leitao
@ 2023-07-14 20:50 ` Alison Schofield
2023-07-15 3:16 ` Dan Williams
2 siblings, 0 replies; 6+ messages in thread
From: Alison Schofield @ 2023-07-14 20:50 UTC (permalink / raw)
To: Breno Leitao
Cc: vishal.l.verma, ira.weiny, bwidawsk, dan.j.williams, linux-cxl,
dave.jiang
On Fri, Jul 14, 2023 at 02:31:45AM -0700, Breno Leitao wrote:
> KASAN and KFENCE detected an user-after-free in the CXL driver. This
> happens in the cxl_decoder_add() fail path. KASAN prints the following
> error:
>
> BUG: KASAN: slab-use-after-free in cxl_parse_cfmws (drivers/cxl/acpi.c:299)
>
> This is happening in cxl_parse_cfmws(), where put_device() is called,
> releasing cxld, which is accessed later.
>
> Just use the local variables in the dev_err() instead of pointing to the
> released memory.
>
> Fixes: e50fe01e1f2a ("cxl/core: Drop ->platform_res attribute for root decoders")
> Signed-off-by: Breno Leitao <leitao@debian.org>
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> ---
> v1 -> v2
> * Return the error (rc) instead of swalling it
> v2 -> v3
> * Split the change in two patches
> * Fix the reference instead of the order
>
> drivers/cxl/acpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 658e6b84a769..642983da01cb 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -297,7 +297,7 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
> rc = cxl_decoder_autoremove(dev, cxld);
> if (rc) {
> dev_err(dev, "Failed to add decode range [%#llx - %#llx]\n",
> - cxld->hpa_range.start, cxld->hpa_range.end);
> + res->start, res->end);
> return 0;
> }
> dev_dbg(dev, "add: %s node: %d range [%#llx - %#llx]\n",
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH v3 1/2] cxl/acpi: Fix UAF in the error path
2023-07-14 9:31 [PATCH v3 1/2] cxl/acpi: Fix UAF in the error path Breno Leitao
2023-07-14 9:31 ` [PATCH v3 2/2] cxl/acpi: Do not swallow errors Breno Leitao
2023-07-14 20:50 ` [PATCH v3 1/2] cxl/acpi: Fix UAF in the error path Alison Schofield
@ 2023-07-15 3:16 ` Dan Williams
2 siblings, 0 replies; 6+ messages in thread
From: Dan Williams @ 2023-07-15 3:16 UTC (permalink / raw)
To: Breno Leitao, alison.schofield, vishal.l.verma, ira.weiny,
bwidawsk, dan.j.williams
Cc: linux-cxl, dave.jiang
Breno Leitao wrote:
> KASAN and KFENCE detected an user-after-free in the CXL driver. This
> happens in the cxl_decoder_add() fail path. KASAN prints the following
> error:
>
> BUG: KASAN: slab-use-after-free in cxl_parse_cfmws (drivers/cxl/acpi.c:299)
>
> This is happening in cxl_parse_cfmws(), where put_device() is called,
> releasing cxld, which is accessed later.
>
> Just use the local variables in the dev_err() instead of pointing to the
> released memory.
>
> Fixes: e50fe01e1f2a ("cxl/core: Drop ->platform_res attribute for root decoders")
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> v1 -> v2
> * Return the error (rc) instead of swalling it
> v2 -> v3
> * Split the change in two patches
> * Fix the reference instead of the order
>
> drivers/cxl/acpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 658e6b84a769..642983da01cb 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -297,7 +297,7 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
> rc = cxl_decoder_autoremove(dev, cxld);
> if (rc) {
> dev_err(dev, "Failed to add decode range [%#llx - %#llx]\n",
> - cxld->hpa_range.start, cxld->hpa_range.end);
> + res->start, res->end);
Came here after the 0day report...
Since this is switching the reuse @res, it can also switch to using %pr
to print the resource.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-15 3:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-14 9:31 [PATCH v3 1/2] cxl/acpi: Fix UAF in the error path Breno Leitao
2023-07-14 9:31 ` [PATCH v3 2/2] cxl/acpi: Do not swallow errors Breno Leitao
2023-07-14 20:50 ` Alison Schofield
2023-07-14 22:33 ` Verma, Vishal L
2023-07-14 20:50 ` [PATCH v3 1/2] cxl/acpi: Fix UAF in the error path Alison Schofield
2023-07-15 3:16 ` Dan Williams
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox