From: huhai <[email protected]>
When scpi probe fails, do not just return the error code, but also reset
the global scpi_info to NULL, otherwise scpi_hwmon_probe() may get a UAF
and cause panic:
scpi_protocol FTSC0001:00: incorrect or no SCP firmware found
... ...
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
Mem abort info:
ESR = 0x86000005
Exception class = IABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
user pgtable: 64k pages, 48-bit VAs, pgdp = (____ptrval____)
[0000000000000000] pgd=0000000000000000, pud=0000000000000000
Internal error: Oops: 86000005 [#1] SMP
... ...
pc : (null)
lr : scpi_hwmon_probe+0x2c/0x4bc [scpi_hwmon]
sp : ffff801fa13cfb20
x29: ffff801fa13cfb20 x28: ffff00000931c1e8
x27: ffff0000093b7318 x26: 000000000000000d
x25: ffff801fa1174da8 x24: 0000000000000000
x23: ffff801fa15e9000 x22: 0000000000000000
x21: ffff0000012c0028 x20: ffff801fa15e9010
x19: 0000000000000000 x18: 0000000000000000
x17: 0000000000000000 x16: 0000000000000000
x15: 0000000000000400 x14: 0000000000000400
x13: 0000000000000001 x12: 0000000000000018
x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
x9 : fefefefeff2f2f39 x8 : 7f7f7f7f7f7f7f7f
x7 : 302f2f2f52525345 x6 : ffff801fa5771a8c
x5 : 0000000000000000 x4 : 0000000000000000
x3 : ffff801fa13bbc00 x2 : ffff000009360118
x1 : 0000000000000000 x0 : ffff801fa13cfbbe
Call trace:
(null)
platform_drv_probe+0x50/0xa0
really_probe+0x240/0x420
driver_probe_device+0x68/0x134
__device_attach_driver+0xb8/0x130
bus_for_each_drv+0x64/0xa0
__device_attach+0xa8/0x194
device_initial_probe+0x10/0x1c
bus_probe_device+0x90/0xa0
deferred_probe_work_func+0x90/0xe0
process_one_work+0x1c0/0x390
worker_thread+0x6c/0x384
kthread+0xfc/0x12c
ret_from_fork+0x10/0x18
Code: bad PC value
---[ end trace b4b2f27a69b5712c ]---
Reported-by: k2ci <[email protected]>
Signed-off-by: huhai <[email protected]>
---
drivers/firmware/arm_scpi.c | 36 +++++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index ddf0b9ff9e15..ad2355814bdf 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -924,17 +924,20 @@ static int scpi_probe(struct platform_device *pdev)
count = of_count_phandle_with_args(np, "mboxes", "#mbox-cells");
if (count < 0) {
dev_err(dev, "no mboxes property in '%pOF'\n", np);
- return -ENODEV;
+ ret = -ENODEV;
+ goto err;
}
scpi_info->channels = devm_kcalloc(dev, count, sizeof(struct scpi_chan),
GFP_KERNEL);
- if (!scpi_info->channels)
- return -ENOMEM;
+ if (!scpi_info->channels) {
+ ret = -ENOMEM;
+ goto err;
+ }
ret = devm_add_action(dev, scpi_free_channels, scpi_info);
if (ret)
- return ret;
+ goto err;
for (; scpi_info->num_chans < count; scpi_info->num_chans++) {
resource_size_t size;
@@ -943,21 +946,24 @@ static int scpi_probe(struct platform_device *pdev)
struct mbox_client *cl = &pchan->cl;
struct device_node *shmem = of_parse_phandle(np, "shmem", idx);
- if (!of_match_node(shmem_of_match, shmem))
- return -ENXIO;
+ if (!of_match_node(shmem_of_match, shmem)) {
+ ret = -ENXIO;
+ goto err;
+ }
ret = of_address_to_resource(shmem, 0, &res);
of_node_put(shmem);
if (ret) {
dev_err(dev, "failed to get SCPI payload mem resource\n");
- return ret;
+ goto err;
}
size = resource_size(&res);
pchan->rx_payload = devm_ioremap(dev, res.start, size);
if (!pchan->rx_payload) {
dev_err(dev, "failed to ioremap SCPI payload\n");
- return -EADDRNOTAVAIL;
+ ret = -EADDRNOTAVAIL;
+ goto err;
}
pchan->tx_payload = pchan->rx_payload + (size >> 1);
@@ -983,7 +989,7 @@ static int scpi_probe(struct platform_device *pdev)
dev_err(dev, "failed to get channel%d err %d\n",
idx, ret);
}
- return ret;
+ goto err;
}
scpi_info->commands = scpi_std_commands;
@@ -1004,7 +1010,7 @@ static int scpi_probe(struct platform_device *pdev)
ret = scpi_init_versions(scpi_info);
if (ret) {
dev_err(dev, "incorrect or no SCP firmware found\n");
- return ret;
+ goto err;
}
if (scpi_info->is_legacy && !scpi_info->protocol_version &&
@@ -1024,7 +1030,15 @@ static int scpi_probe(struct platform_device *pdev)
scpi_info->firmware_version));
scpi_info->scpi_ops = &scpi_ops;
- return devm_of_platform_populate(dev);
+ ret = devm_of_platform_populate(dev);
+ if (ret)
+ goto err;
+
+ return ret;
+err:
+ /* stop exporting SCPI ops through get_scpi_ops */
+ scpi_info = NULL;
+ return ret;
}
static const struct of_device_id scpi_of_match[] = {
--
2.27.0
On Fri, Jul 01, 2022 at 02:16:06PM +0800, huhai wrote:
> From: huhai <[email protected]>
>
> When scpi probe fails, do not just return the error code, but also reset
> the global scpi_info to NULL, otherwise scpi_hwmon_probe() may get a UAF
> and cause panic:
>
Interesting, which platform are you using this on ? SCPI is deprecated for
a while, please switch to SCMI which is well maintained both in terms of
specification and support in the kernel. I also assume this is 64-bit
platform, so I don't want you to get stuck in future because of lack
of some feature in SCPI. Please see if you can migrate to SCMI.
> scpi_protocol FTSC0001:00: incorrect or no SCP firmware found
> ... ...
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> Mem abort info:
>
[...]
I think we don't see to make it complex. Can't it be as simple as:
Regards,
Sudeep
-->8
diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
index ddf0b9ff9e15..6fa1a5b193b8 100644
--- i/drivers/firmware/arm_scpi.c
+++ w/drivers/firmware/arm_scpi.c
@@ -799,7 +799,7 @@ static struct scpi_ops scpi_ops = {
struct scpi_ops *get_scpi_ops(void)
{
- return scpi_info ? scpi_info->scpi_ops : NULL;
+ return scpi_info && scpi_info->scpi_ops ? scpi_info->scpi_ops : NULL;
}
EXPORT_SYMBOL_GPL(get_scpi_ops);
>On Fri, Jul 01, 2022 at 02:16:06PM +0800, huhai wrote:
>> From: huhai <[email protected]>
>>
>> When scpi probe fails, do not just return the error code, but also reset
>> the global scpi_info to NULL, otherwise scpi_hwmon_probe() may get a UAF
>> and cause panic:
>>
>
>Interesting, which platform are you using this on ? SCPI is deprecated for
>a while, please switch to SCMI which is well maintained both in terms of
>specification and support in the kernel. I also assume this is 64-bit
>platform, so I don't want you to get stuck in future because of lack
>of some feature in SCPI. Please see if you can migrate to SCMI.
>
>> scpi_protocol FTSC0001:00: incorrect or no SCP firmware found
>> ... ...
>> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
>> Mem abort info:
>>
>
>[...]
>
>I think we don't see to make it complex. Can't it be as simple as:
>
>Regards,
>Sudeep
>
>-->8
>
>diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
>index ddf0b9ff9e15..6fa1a5b193b8 100644
>--- i/drivers/firmware/arm_scpi.c
>+++ w/drivers/firmware/arm_scpi.c
>@@ -799,7 +799,7 @@ static struct scpi_ops scpi_ops = {
>
> struct scpi_ops *get_scpi_ops(void)
> {
>- return scpi_info ? scpi_info->scpi_ops : NULL;
>+ return scpi_info && scpi_info->scpi_ops ? scpi_info->scpi_ops : NULL;
I don't think it work well, because it's a UAF and scpi_info->scpi_ops could be any value set by others.
> }
> EXPORT_SYMBOL_GPL(get_scpi_ops);
On Fri, Jul 01, 2022 at 10:42:12AM +0100, Sudeep Holla wrote:
> diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
> index ddf0b9ff9e15..6fa1a5b193b8 100644
> --- i/drivers/firmware/arm_scpi.c
> +++ w/drivers/firmware/arm_scpi.c
> @@ -799,7 +799,7 @@ static struct scpi_ops scpi_ops = {
>
> struct scpi_ops *get_scpi_ops(void)
> {
> - return scpi_info ? scpi_info->scpi_ops : NULL;
> + return scpi_info && scpi_info->scpi_ops ? scpi_info->scpi_ops : NULL;
Scratch that, scpi_info->scpi_ops will be NULL and it is fine to return NULL.
Only reason why something can go wrong is if devm_of_platform_populate()
fails and we return error after setting up scpi_info->scpi_ops.
Can you check if the below fixes the issue ? We can continue working as
a driver even if few of the devices were populated which could be the case
but I am still doubtful about that.
Regards,
Sudeep
-->8
diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
index ddf0b9ff9e15..bae2e18e24ee 100644
--- i/drivers/firmware/arm_scpi.c
+++ w/drivers/firmware/arm_scpi.c
@@ -1024,7 +1024,9 @@ static int scpi_probe(struct platform_device *pdev)
scpi_info->firmware_version));
scpi_info->scpi_ops = &scpi_ops;
- return devm_of_platform_populate(dev);
+ devm_of_platform_populate(dev);
+
+ return ret;
}
>On Fri, Jul 01, 2022 at 02:16:06PM +0800, huhai wrote:
>> From: huhai <[email protected]>
>>
>> When scpi probe fails, do not just return the error code, but also reset
>> the global scpi_info to NULL, otherwise scpi_hwmon_probe() may get a UAF
>> and cause panic:
>>
>
>Interesting, which platform are you using this on ? SCPI is deprecated for
>a while, please switch to SCMI which is well maintained both in terms of
>specification and support in the kernel. I also assume this is 64-bit
>platform, so I don't want you to get stuck in future because of lack
>of some feature in SCPI. Please see if you can migrate to SCMI.
Thinks for sharing this massage, and my platform is FT2000+(ARM64).
>
>> scpi_protocol FTSC0001:00: incorrect or no SCP firmware found
>> ... ...
>> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
>> Mem abort info:
>>
>
>[...]
>
>I think we don't see to make it complex. Can't it be as simple as:
scpi_info is a global variable?? we must set it to NULL when it is not valid
>Regards,
>Sudeep
>
>-->8
>
>diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
>index ddf0b9ff9e15..6fa1a5b193b8 100644
>--- i/drivers/firmware/arm_scpi.c
>+++ w/drivers/firmware/arm_scpi.c
>@@ -799,7 +799,7 @@ static struct scpi_ops scpi_ops = {
>
> struct scpi_ops *get_scpi_ops(void)
> {
>- return scpi_info ? scpi_info->scpi_ops : NULL;
>+ return scpi_info && scpi_info->scpi_ops ? scpi_info->scpi_ops : NULL;
I don't think it work well, because it's a UAF and scpi_info->scpi_ops could be any value set by others.
> }
> EXPORT_SYMBOL_GPL(get_scpi_ops);
On Fri, Jul 01, 2022 at 06:04:53PM +0800, huhai wrote:
>
> scpi_info is a global variableļ¼ we must set it to NULL when it is not valid
How about not assigning the global variable until the end of the probe ?
Something like below:
Regards,
Sudeep
-->8
diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
index ddf0b9ff9e15..5463501735ff 100644
--- i/drivers/firmware/arm_scpi.c
+++ w/drivers/firmware/arm_scpi.c
@@ -913,13 +913,14 @@ static int scpi_probe(struct platform_device *pdev)
struct resource res;
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
+ struct scpi_drvinfo *scpi_drvinfo;
- scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
- if (!scpi_info)
+ scpi_drvinfo = devm_kzalloc(dev, sizeof(*scpi_drvinfo), GFP_KERNEL);
+ if (!scpi_drvinfo)
return -ENOMEM;
if (of_match_device(legacy_scpi_of_match, &pdev->dev))
- scpi_info->is_legacy = true;
+ scpi_drvinfo->is_legacy = true;
count = of_count_phandle_with_args(np, "mboxes", "#mbox-cells");
if (count < 0) {
@@ -927,19 +928,19 @@ static int scpi_probe(struct platform_device *pdev)
return -ENODEV;
}
- scpi_info->channels = devm_kcalloc(dev, count, sizeof(struct scpi_chan),
- GFP_KERNEL);
- if (!scpi_info->channels)
+ scpi_drvinfo->channels =
+ devm_kcalloc(dev, count, sizeof(struct scpi_chan), GFP_KERNEL);
+ if (!scpi_drvinfo->channels)
return -ENOMEM;
- ret = devm_add_action(dev, scpi_free_channels, scpi_info);
+ ret = devm_add_action(dev, scpi_free_channels, scpi_drvinfo);
if (ret)
return ret;
- for (; scpi_info->num_chans < count; scpi_info->num_chans++) {
+ for (; scpi_drvinfo->num_chans < count; scpi_drvinfo->num_chans++) {
resource_size_t size;
- int idx = scpi_info->num_chans;
- struct scpi_chan *pchan = scpi_info->channels + idx;
+ int idx = scpi_drvinfo->num_chans;
+ struct scpi_chan *pchan = scpi_drvinfo->channels + idx;
struct mbox_client *cl = &pchan->cl;
struct device_node *shmem = of_parse_phandle(np, "shmem", idx);
@@ -986,43 +987,44 @@ static int scpi_probe(struct platform_device *pdev)
return ret;
}
- scpi_info->commands = scpi_std_commands;
+ scpi_drvinfo->commands = scpi_std_commands;
- platform_set_drvdata(pdev, scpi_info);
+ platform_set_drvdata(pdev, scpi_drvinfo);
- if (scpi_info->is_legacy) {
+ if (scpi_drvinfo->is_legacy) {
/* Replace with legacy variants */
scpi_ops.clk_set_val = legacy_scpi_clk_set_val;
- scpi_info->commands = scpi_legacy_commands;
+ scpi_drvinfo->commands = scpi_legacy_commands;
/* Fill priority bitmap */
for (idx = 0; idx < ARRAY_SIZE(legacy_hpriority_cmds); idx++)
set_bit(legacy_hpriority_cmds[idx],
- scpi_info->cmd_priority);
+ scpi_drvinfo->cmd_priority);
}
- ret = scpi_init_versions(scpi_info);
+ ret = scpi_init_versions(scpi_drvinfo);
if (ret) {
dev_err(dev, "incorrect or no SCP firmware found\n");
return ret;
}
- if (scpi_info->is_legacy && !scpi_info->protocol_version &&
- !scpi_info->firmware_version)
+ if (scpi_drvinfo->is_legacy && !scpi_drvinfo->protocol_version &&
+ !scpi_drvinfo->firmware_version)
dev_info(dev, "SCP Protocol legacy pre-1.0 firmware\n");
else
dev_info(dev, "SCP Protocol %lu.%lu Firmware %lu.%lu.%lu version\n",
FIELD_GET(PROTO_REV_MAJOR_MASK,
- scpi_info->protocol_version),
+ scpi_drvinfo->protocol_version),
FIELD_GET(PROTO_REV_MINOR_MASK,
- scpi_info->protocol_version),
+ scpi_drvinfo->protocol_version),
FIELD_GET(FW_REV_MAJOR_MASK,
- scpi_info->firmware_version),
+ scpi_drvinfo->firmware_version),
FIELD_GET(FW_REV_MINOR_MASK,
- scpi_info->firmware_version),
+ scpi_drvinfo->firmware_version),
FIELD_GET(FW_REV_PATCH_MASK,
- scpi_info->firmware_version));
- scpi_info->scpi_ops = &scpi_ops;
+ scpi_drvinfo->firmware_version));
+ scpi_drvinfo->scpi_ops = &scpi_ops;
+ scpi_info = scpi_drvinfo;
return devm_of_platform_populate(dev);
}
>On Fri, Jul 01, 2022 at 06:04:53PM +0800, huhai wrote:
>>
>> scpi_info is a global variable?? we must set it to NULL when it is not valid
>
>How about not assigning the global variable until the end of the probe ?
>Something like below:>
>Regards,
>Sudeep
>
>-->8
>
>diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
>index ddf0b9ff9e15..5463501735ff 100644
>--- i/drivers/firmware/arm_scpi.c
>+++ w/drivers/firmware/arm_scpi.c
>@@ -913,13 +913,14 @@ static int scpi_probe(struct platform_device *pdev)
> struct resource res;
> struct device *dev = &pdev->dev;
> struct device_node *np = dev->of_node;
>+ struct scpi_drvinfo *scpi_drvinfo;
>
>- scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
>- if (!scpi_info)
>+ scpi_drvinfo = devm_kzalloc(dev, sizeof(*scpi_drvinfo), GFP_KERNEL);
>+ if (!scpi_drvinfo)
> return -ENOMEM;
>
> if (of_match_device(legacy_scpi_of_match, &pdev->dev))
>- scpi_info->is_legacy = true;
>+ scpi_drvinfo->is_legacy = true;
>
> count = of_count_phandle_with_args(np, "mboxes", "#mbox-cells");
> if (count < 0) {
>@@ -927,19 +928,19 @@ static int scpi_probe(struct platform_device *pdev)
> return -ENODEV;
> }
>
>- scpi_info->channels = devm_kcalloc(dev, count, sizeof(struct scpi_chan),
>- GFP_KERNEL);
>- if (!scpi_info->channels)
>+ scpi_drvinfo->channels =
>+ devm_kcalloc(dev, count, sizeof(struct scpi_chan), GFP_KERNEL);
>+ if (!scpi_drvinfo->channels)
> return -ENOMEM;
>
>- ret = devm_add_action(dev, scpi_free_channels, scpi_info);
>+ ret = devm_add_action(dev, scpi_free_channels, scpi_drvinfo);
> if (ret)
> return ret;
>
>- for (; scpi_info->num_chans < count; scpi_info->num_chans++) {
>+ for (; scpi_drvinfo->num_chans < count; scpi_drvinfo->num_chans++) {
> resource_size_t size;
>- int idx = scpi_info->num_chans;
>- struct scpi_chan *pchan = scpi_info->channels + idx;
>+ int idx = scpi_drvinfo->num_chans;
>+ struct scpi_chan *pchan = scpi_drvinfo->channels + idx;
> struct mbox_client *cl = &pchan->cl;
> struct device_node *shmem = of_parse_phandle(np, "shmem", idx);
>
>@@ -986,43 +987,44 @@ static int scpi_probe(struct platform_device *pdev)
> return ret;
> }
>
>- scpi_info->commands = scpi_std_commands;
>+ scpi_drvinfo->commands = scpi_std_commands;
>
>- platform_set_drvdata(pdev, scpi_info);
>+ platform_set_drvdata(pdev, scpi_drvinfo);
>
>- if (scpi_info->is_legacy) {
>+ if (scpi_drvinfo->is_legacy) {
> /* Replace with legacy variants */
> scpi_ops.clk_set_val = legacy_scpi_clk_set_val;
>- scpi_info->commands = scpi_legacy_commands;
>+ scpi_drvinfo->commands = scpi_legacy_commands;
>
> /* Fill priority bitmap */
> for (idx = 0; idx < ARRAY_SIZE(legacy_hpriority_cmds); idx++)
> set_bit(legacy_hpriority_cmds[idx],
>- scpi_info->cmd_priority);
>+ scpi_drvinfo->cmd_priority);
> }
>
>- ret = scpi_init_versions(scpi_info);
>+ ret = scpi_init_versions(scpi_drvinfo);
> if (ret) {
> dev_err(dev, "incorrect or no SCP firmware found\n");
> return ret;
> }
>
>- if (scpi_info->is_legacy && !scpi_info->protocol_version &&
>- !scpi_info->firmware_version)
>+ if (scpi_drvinfo->is_legacy && !scpi_drvinfo->protocol_version &&
>+ !scpi_drvinfo->firmware_version)
> dev_info(dev, "SCP Protocol legacy pre-1.0 firmware\n");
> else
> dev_info(dev, "SCP Protocol %lu.%lu Firmware %lu.%lu.%lu version\n",
> FIELD_GET(PROTO_REV_MAJOR_MASK,
>- scpi_info->protocol_version),
>+ scpi_drvinfo->protocol_version),
> FIELD_GET(PROTO_REV_MINOR_MASK,
>- scpi_info->protocol_version),
>+ scpi_drvinfo->protocol_version),
> FIELD_GET(FW_REV_MAJOR_MASK,
>- scpi_info->firmware_version),
>+ scpi_drvinfo->firmware_version),
> FIELD_GET(FW_REV_MINOR_MASK,
>- scpi_info->firmware_version),
>+ scpi_drvinfo->firmware_version),
> FIELD_GET(FW_REV_PATCH_MASK,
>- scpi_info->firmware_version));
>- scpi_info->scpi_ops = &scpi_ops;
>+ scpi_drvinfo->firmware_version));
>+ scpi_drvinfo->scpi_ops = &scpi_ops;
>+ scpi_info = scpi_drvinfo;
Yes, I think this patch will work well until it runs here.
>
> return devm_of_platform_populate(dev);
we should not return devm_of_platform_populate() directly, because devm_of_platform_populate()
may fails, if it fails, the scpi_info will pointing to free-ed memory.
as you said before:
- return devm_of_platform_populate(dev);
+ devm_of_platform_populate(dev);
+
+ return ret;
will work well.
thanks
> }