Re: [PATCH] firmware: arm_scpi: Ensure scpi_info is not assigned if the probe fails

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[] = {