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

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

[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.

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jackie Liu]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel