On Tue, 22 Oct 2024 at 07:43, Kuldeep Singh <quic_kuldsing@xxxxxxxxxxx> wrote: > > > > On 10/16/2024 2:31 PM, Kuldeep Singh wrote: > > > > On 10/14/2024 6:38 PM, Bartosz Golaszewski wrote: > >> On Mon, Oct 14, 2024 at 1:19 PM Kuldeep Singh <quic_kuldsing@xxxxxxxxxxx> wrote: > >>> > >>> The qcom_tzmem driver currently has exposed APIs that lack validations > >>> on required input parameters. This oversight can lead to unexpected null > >>> pointer dereference crashes. > >>> > >> > >> The commit message is not true. None of the things you changed below > >> can lead to a NULL-pointer dereference.> > >>> To address this issue, add sanity for required input parameters. > >>> > >>> Signed-off-by: Kuldeep Singh <quic_kuldsing@xxxxxxxxxxx> > >>> --- > >>> drivers/firmware/qcom/qcom_tzmem.c | 6 ++++++ > >>> 1 file changed, 6 insertions(+) > >>> > >>> diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c > >>> index 92b365178235..977e48fec32f 100644 > >>> --- a/drivers/firmware/qcom/qcom_tzmem.c > >>> +++ b/drivers/firmware/qcom/qcom_tzmem.c > >>> @@ -203,6 +203,9 @@ qcom_tzmem_pool_new(const struct qcom_tzmem_pool_config *config) > >>> > >>> might_sleep(); > >>> > >>> + if (!config->policy) > >>> + return ERR_PTR(-EINVAL); > >> > >> This is already handled by the default case of the switch. > > > > Ack. Need to drop. > > https://elixir.bootlin.com/linux/v6.12-rc3/source/drivers/firmware/qcom/qcom_tzmem.c#L218 > > > > While examining qcom_tzmem_pool_free under the same principle, it > > appears the following check is unnecessary. > > https://elixir.bootlin.com/linux/v6.12-rc3/source/drivers/firmware/qcom/qcom_tzmem.c#L268 > > > > Bartosz, > I am thinking to remove below check in next rev like mentioned above. > https://elixir.bootlin.com/linux/v6.12-rc3/source/drivers/firmware/qcom/qcom_tzmem.c#L268 > > Do you have any other opinion here? > Please let me know. > No, let's keep the NULL-pointer check and add it to qcom_tzmem_free(), I'm not against it. I was just saying that in the latter case it will already be handled by the radix tree lookup. Bart
