| Message ID | 20220701160310.148344-1-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: arm_scpi: Ensure scpi_info is not assigned if the probe fails | expand |
Hi Sudeep. Thanks for your patch, It's look good to me. Reviewed-by: Jackie Liu <liuyun01@kylinos.cn> 在 2022/7/2 00:03, Sudeep Holla 写道: > When scpi probe fails, at any point, we need to ensure that the scpi_info > is not set and will remain NULL until the probe succeeds. If it is not > taken care, then it could result in kernel panic with a NULL pointer > dereference. I think the null pointer reference is not correct. It should be UAF. The logic is as follows: scpi_info = devm_zalloc After that if fails, the address will be released, but scpi_info is not NULL. Normal, there will be no problem, because scpi_info is alloc by kzalloc, so even if scpi_info is not NULL, but scpi_info->scpi_ops is NULL, It still work normally. But if another process or thread alloc a new data, if they are same address, and then it is assigned a value, so wild pointer scpi_info->scpi_ops is not NULL now, Then, Panic. Thanks. -- Jackie Liu. > > Reported-by: huhai <huhai@kylinos.cn> > Cc: stable@vger.kernel.org # 4.19+ > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_scpi.c | 57 +++++++++++++++++++++---------------- > 1 file changed, 32 insertions(+), 25 deletions(-) > > diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c > index ddf0b9ff9e15..085a71a00171 100644 > --- a/drivers/firmware/arm_scpi.c > +++ b/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,45 +987,51 @@ 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; > + > + ret = devm_of_platform_populate(dev); > > - return devm_of_platform_populate(dev); > + if (!ret) > + scpi_info = scpi_drvinfo; > + > + return ret; > } > > static const struct of_device_id scpi_of_match[] = {
On Mon, Jul 04, 2022 at 09:19:56AM +0800, Jackie Liu wrote: > Hi Sudeep. > > Thanks for your patch, It's look good to me. > > Reviewed-by: Jackie Liu <liuyun01@kylinos.cn> > > 在 2022/7/2 00:03, Sudeep Holla 写道: > > When scpi probe fails, at any point, we need to ensure that the scpi_info > > is not set and will remain NULL until the probe succeeds. If it is not > > taken care, then it could result in kernel panic with a NULL pointer > > dereference. > > I think the null pointer reference is not correct. It should be UAF. The > logic is as follows: > Right, I will update the commit message, sorry for that got carried away by the message in the kernel panic. > scpi_info = devm_zalloc > > After that if fails, the address will be released, but scpi_info is not > NULL. Normal, there will be no problem, because scpi_info is alloc by > kzalloc, so even if scpi_info is not NULL, but scpi_info->scpi_ops is > NULL, It still work normally. > > But if another process or thread alloc a new data, if they are same address, > and then it is assigned a value, so wild pointer scpi_info->scpi_ops is not > NULL now, Then, Panic. > I do understand that, I will update the commit log to cover these and thanks for the review.
On Fri, 1 Jul 2022 17:03:10 +0100, Sudeep Holla wrote: > When scpi probe fails, at any point, we need to ensure that the scpi_info > is not set and will remain NULL until the probe succeeds. If it is not > taken care, then it could result in kernel panic with a NULL pointer > dereference. > > Applied to sudeep.holla/linux (for-next/scmi), thanks! [1/1] firmware: arm_scpi: Ensure scpi_info is not assigned if the probe fails https://git.kernel.org/sudeep.holla/c/689640efc0 -- Regards, Sudeep
diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index ddf0b9ff9e15..085a71a00171 100644 --- a/drivers/firmware/arm_scpi.c +++ b/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,45 +987,51 @@ 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; + + ret = devm_of_platform_populate(dev); - return devm_of_platform_populate(dev); + if (!ret) + scpi_info = scpi_drvinfo; + + return ret; } static const struct of_device_id scpi_of_match[] = {
When scpi probe fails, at any point, we need to ensure that the scpi_info is not set and will remain NULL until the probe succeeds. If it is not taken care, then it could result in kernel panic with a NULL pointer dereference. Reported-by: huhai <huhai@kylinos.cn> Cc: stable@vger.kernel.org # 4.19+ Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/firmware/arm_scpi.c | 57 +++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 25 deletions(-)