[PATCH] pinctrl: rockchip: fix NULL ptr deref in rockchip_pinctrl_parse_groups()

[PATCH] pinctrl: rockchip: fix NULL ptr deref in rockchip_pinctrl_parse_groups()

[Sergey Shtylyov]

In the Rockchip driver, rockchip_pinctrl_parse_groups() assumes that the
"rockchip,pins" property will always be present in the DT node it parses
and so doesn't check the result of of_get_property() for NULL. If the DT
passed to the kernel happens to have such property missing, then we will
get a kernel oops when the pointer is dereferenced in the *for* loop just
a few lines after the call.  I think it's better to play safe by checking
the list variable for NULL (and reporting error if so), like we check the
size variable for validity further down...

Found by Linux Verification Center (linuxtesting.org) with the Svace static
analysis tool.

Fixes: d3e5116119bd ("pinctrl: add pinctrl driver for Rockchip SoCs")
Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

---
The patch is against the master branch of Linus Torvalds' linux.git repo.

 drivers/pinctrl/pinctrl-rockchip.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux/drivers/pinctrl/pinctrl-rockchip.c
===================================================================
--- linux.orig/drivers/pinctrl/pinctrl-rockchip.c
+++ linux/drivers/pinctrl/pinctrl-rockchip.c
@@ -3488,7 +3488,9 @@ static int rockchip_pinctrl_parse_groups
 	 * do sanity check and calculate pins number
 	 */
 	list = of_get_property(np, "rockchip,pins", &size);
-	/* we do not check return since it's safe node passed down */
+	if (!list)
+		return dev_err_probe(dev, -EINVAL,
+				     "%pOF: no rockchip,pins property\n", np);
 	size /= sizeof(*list);
 	if (!size || size % 4)
 		return dev_err_probe(dev, -EINVAL,

Re: [PATCH] pinctrl: rockchip: fix NULL ptr deref in rockchip_pinctrl_parse_groups()

[Heiko Stuebner]

Am Mittwoch, 3. September 2025, 21:48:54 Mitteleuropäische Sommerzeit schrieb Sergey Shtylyov:
> In the Rockchip driver, rockchip_pinctrl_parse_groups() assumes that the
> "rockchip,pins" property will always be present in the DT node it parses
> and so doesn't check the result of of_get_property() for NULL. If the DT
> passed to the kernel happens to have such property missing, then we will
> get a kernel oops when the pointer is dereferenced in the *for* loop just
> a few lines after the call.  I think it's better to play safe by checking
> the list variable for NULL (and reporting error if so), like we check the
> size variable for validity further down...
> 
> Found by Linux Verification Center (linuxtesting.org) with the Svace static
> analysis tool.
> 
> Fixes: d3e5116119bd ("pinctrl: add pinctrl driver for Rockchip SoCs")
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

Assuming that the DT is our friend, really is a bad assumption :-) .

While I can't imagine what 12-year-ago-me was thinking then, simply
checking the return value really is the better way

Reviewed-by: Heiko Stuebner <heiko@sntech.de>


> 
> ---
> The patch is against the master branch of Linus Torvalds' linux.git repo.
> 
>  drivers/pinctrl/pinctrl-rockchip.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Index: linux/drivers/pinctrl/pinctrl-rockchip.c
> ===================================================================
> --- linux.orig/drivers/pinctrl/pinctrl-rockchip.c
> +++ linux/drivers/pinctrl/pinctrl-rockchip.c
> @@ -3488,7 +3488,9 @@ static int rockchip_pinctrl_parse_groups
>  	 * do sanity check and calculate pins number
>  	 */
>  	list = of_get_property(np, "rockchip,pins", &size);
> -	/* we do not check return since it's safe node passed down */
> +	if (!list)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "%pOF: no rockchip,pins property\n", np);
>  	size /= sizeof(*list);
>  	if (!size || size % 4)
>  		return dev_err_probe(dev, -EINVAL,
> 

Re: [PATCH] pinctrl: rockchip: fix NULL ptr deref in rockchip_pinctrl_parse_groups()

[Chen-Yu Tsai]

On Mon, Sep 8, 2025 at 1:32 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
> Am Mittwoch, 3. September 2025, 21:48:54 Mitteleuropäische Sommerzeit schrieb Sergey Shtylyov:
> > In the Rockchip driver, rockchip_pinctrl_parse_groups() assumes that the
> > "rockchip,pins" property will always be present in the DT node it parses
> > and so doesn't check the result of of_get_property() for NULL. If the DT
> > passed to the kernel happens to have such property missing, then we will
> > get a kernel oops when the pointer is dereferenced in the *for* loop just
> > a few lines after the call.  I think it's better to play safe by checking
> > the list variable for NULL (and reporting error if so), like we check the
> > size variable for validity further down...
> >
> > Found by Linux Verification Center (linuxtesting.org) with the Svace static
> > analysis tool.
> >
> > Fixes: d3e5116119bd ("pinctrl: add pinctrl driver for Rockchip SoCs")
> > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> Assuming that the DT is our friend, really is a bad assumption :-) .

If this is invalid, perhaps you should make the "rockchip,pins" property
required in the binding?

> While I can't imagine what 12-year-ago-me was thinking then, simply
> checking the return value really is the better way

I think some of us have thought that guarding against incorrect DTs
is not what the kernel is supposed to do.

ChenYu

> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
>
>
> >
> > ---
> > The patch is against the master branch of Linus Torvalds' linux.git repo.
> >
> >  drivers/pinctrl/pinctrl-rockchip.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > Index: linux/drivers/pinctrl/pinctrl-rockchip.c
> > ===================================================================
> > --- linux.orig/drivers/pinctrl/pinctrl-rockchip.c
> > +++ linux/drivers/pinctrl/pinctrl-rockchip.c
> > @@ -3488,7 +3488,9 @@ static int rockchip_pinctrl_parse_groups
> >        * do sanity check and calculate pins number
> >        */
> >       list = of_get_property(np, "rockchip,pins", &size);
> > -     /* we do not check return since it's safe node passed down */
> > +     if (!list)
> > +             return dev_err_probe(dev, -EINVAL,
> > +                                  "%pOF: no rockchip,pins property\n", np);
> >       size /= sizeof(*list);
> >       if (!size || size % 4)
> >               return dev_err_probe(dev, -EINVAL,
> >
>
>
>
>
>

Re: [PATCH] pinctrl: rockchip: fix NULL ptr deref in rockchip_pinctrl_parse_groups()

[Sergey Shtylyov]

On 9/8/25 8:53 AM, Chen-Yu Tsai wrote:
[...]

>> Am Mittwoch, 3. September 2025, 21:48:54 Mitteleuropäische Sommerzeit schrieb Sergey Shtylyov:
>>> In the Rockchip driver, rockchip_pinctrl_parse_groups() assumes that the
>>> "rockchip,pins" property will always be present in the DT node it parses
>>> and so doesn't check the result of of_get_property() for NULL. If the DT
>>> passed to the kernel happens to have such property missing, then we will
>>> get a kernel oops when the pointer is dereferenced in the *for* loop just
>>> a few lines after the call.  I think it's better to play safe by checking
>>> the list variable for NULL (and reporting error if so), like we check the
>>> size variable for validity further down...
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with the Svace static
>>> analysis tool.
>>>
>>> Fixes: d3e5116119bd ("pinctrl: add pinctrl driver for Rockchip SoCs")
>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>
>> Assuming that the DT is our friend, really is a bad assumption :-) .
> 
> If this is invalid, perhaps you should make the "rockchip,pins" property
> required in the binding?

   Looking at Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt
in 5.10.y, it was marked as required. The modern YAML bindings don't seem to
say that, at least explicitly...

[...]

MBR, Sergey

Re: [PATCH] pinctrl: rockchip: fix NULL ptr deref in rockchip_pinctrl_parse_groups()

[Linus Walleij]

On Wed, Sep 3, 2025 at 9:49 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:

> In the Rockchip driver, rockchip_pinctrl_parse_groups() assumes that the
> "rockchip,pins" property will always be present in the DT node it parses
> and so doesn't check the result of of_get_property() for NULL. If the DT
> passed to the kernel happens to have such property missing, then we will
> get a kernel oops when the pointer is dereferenced in the *for* loop just
> a few lines after the call.  I think it's better to play safe by checking
> the list variable for NULL (and reporting error if so), like we check the
> size variable for validity further down...
>
> Found by Linux Verification Center (linuxtesting.org) with the Svace static
> analysis tool.
>
> Fixes: d3e5116119bd ("pinctrl: add pinctrl driver for Rockchip SoCs")
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

Patch applied!

Yours,
Linus Walleij

Re: [PATCH] pinctrl: rockchip: fix NULL ptr deref in rockchip_pinctrl_parse_groups()

[Sergey Shtylyov]

On 9/8/25 3:59 PM, Linus Walleij wrote:
[...]

>> In the Rockchip driver, rockchip_pinctrl_parse_groups() assumes that the
>> "rockchip,pins" property will always be present in the DT node it parses
>> and so doesn't check the result of of_get_property() for NULL. If the DT
>> passed to the kernel happens to have such property missing, then we will
>> get a kernel oops when the pointer is dereferenced in the *for* loop just
>> a few lines after the call.  I think it's better to play safe by checking
>> the list variable for NULL (and reporting error if so), like we check the
>> size variable for validity further down...
>>
>> Found by Linux Verification Center (linuxtesting.org) with the Svace static
>> analysis tool.
>>
>> Fixes: d3e5116119bd ("pinctrl: add pinctrl driver for Rockchip SoCs")
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> Patch applied!

   Where? I'm not seeing it in any Linus' tree... :-/

> Yours,
> Linus Walleij

MBR, Sergey