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

From: Sudeep Holla <sudeep.holla@arm.com>
To: Jackie Liu <liuyun01@kylinos.cn>
Cc: linux-arm-kernel@lists.infradead.org, luriwen@kylinos.cn,
	15815827059@163.com, cristian.marussi@arm.com,
	huhai <huhai@kylinos.cn>,
	stable@vger.kernel.org
Subject: Re: [PATCH] firmware: arm_scpi: Ensure scpi_info is not assigned if the probe fails
Date: Mon, 4 Jul 2022 10:22:43 +0100	[thread overview]
Message-ID: <20220704092243.rzk6l3rhjul4wavq@bogus> (raw)
In-Reply-To: <bf21d317-8d4b-e237-86b7-be577b5bc652@kylinos.cn>

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