[PATCH next] spi: Fix potential uninitialized variable in probe()

[PATCH next] spi: Fix potential uninitialized variable in probe()

[Dan Carpenter]

If the device tree is messed up, then potentially the "protocol" string
could potentially be uninitialized.  Add a check to prevent that.

Fixes: 059f545832be ("spi: add support for microchip "soft" spi controller")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/spi/spi-microchip-core-spi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
index b8738190cdcb..e65036cc62f3 100644
--- a/drivers/spi/spi-microchip-core-spi.c
+++ b/drivers/spi/spi-microchip-core-spi.c
@@ -320,6 +320,8 @@ static int mchp_corespi_probe(struct platform_device *pdev)
 	 */
 	ret = of_property_read_string(pdev->dev.of_node, "microchip,protocol-configuration",
 				      &protocol);
+	if (ret)
+		return ret;
 	if (strcmp(protocol, "motorola") != 0)
 		return dev_err_probe(&pdev->dev, -EINVAL,
 				     "CoreSPI: protocol '%s' not supported by this driver\n",
-- 
2.51.0

Re: [PATCH next] spi: Fix potential uninitialized variable in probe()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

On Fri, Nov 21, 2025 at 04:35:01PM +0300, Dan Carpenter wrote:
> If the device tree is messed up, then potentially the "protocol" string
> could potentially be uninitialized.  Add a check to prevent that.
> 
> Fixes: 059f545832be ("spi: add support for microchip "soft" spi controller")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/spi/spi-microchip-core-spi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> index b8738190cdcb..e65036cc62f3 100644
> --- a/drivers/spi/spi-microchip-core-spi.c
> +++ b/drivers/spi/spi-microchip-core-spi.c
> @@ -320,6 +320,8 @@ static int mchp_corespi_probe(struct platform_device *pdev)
>  	 */
>  	ret = of_property_read_string(pdev->dev.of_node, "microchip,protocol-configuration",
>  				      &protocol);
> +	if (ret)
> +		return ret;
>  	if (strcmp(protocol, "motorola") != 0)
>  		return dev_err_probe(&pdev->dev, -EINVAL,
>  				     "CoreSPI: protocol '%s' not supported by this driver\n",

This should probably also complain about not being able to get the
property, otherwise nobody is going to be able to figure out what's
wrong if we actually hit the error case.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH next] spi: Fix potential uninitialized variable in probe()

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1773 bytes --]

On Fri, Nov 21, 2025 at 02:18:49PM +0000, Mark Brown wrote:
> On Fri, Nov 21, 2025 at 04:35:01PM +0300, Dan Carpenter wrote:
> > If the device tree is messed up, then potentially the "protocol" string
> > could potentially be uninitialized.  Add a check to prevent that.
> > 
> > Fixes: 059f545832be ("spi: add support for microchip "soft" spi controller")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >  drivers/spi/spi-microchip-core-spi.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> > index b8738190cdcb..e65036cc62f3 100644
> > --- a/drivers/spi/spi-microchip-core-spi.c
> > +++ b/drivers/spi/spi-microchip-core-spi.c
> > @@ -320,6 +320,8 @@ static int mchp_corespi_probe(struct platform_device *pdev)
> >  	 */
> >  	ret = of_property_read_string(pdev->dev.of_node, "microchip,protocol-configuration",
> >  				      &protocol);
> > +	if (ret)
> > +		return ret;
> >  	if (strcmp(protocol, "motorola") != 0)
> >  		return dev_err_probe(&pdev->dev, -EINVAL,
> >  				     "CoreSPI: protocol '%s' not supported by this driver\n",
> 
> This should probably also complain about not being able to get the
> property, otherwise nobody is going to be able to figure out what's
> wrong if we actually hit the error case.

The one thing to be careful of is that the property has a default, so
EINVAL needs to be treated differently, so the decision tree is
something like:
if (ret == _EINVAL)
	<do nothing>
else if (ret)
	abort complaining about malformed
else if (!motorola)
	abort complaining about unsupported mode
else
	<do nothing>

obviously that can just become two clauses, but you get the idea.




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH next] spi: Fix potential uninitialized variable in probe()

[Dan Carpenter]

On Fri, Nov 21, 2025 at 04:20:41PM +0000, Conor Dooley wrote:
> On Fri, Nov 21, 2025 at 02:18:49PM +0000, Mark Brown wrote:
> > On Fri, Nov 21, 2025 at 04:35:01PM +0300, Dan Carpenter wrote:
> > > If the device tree is messed up, then potentially the "protocol" string
> > > could potentially be uninitialized.  Add a check to prevent that.
> > > 
> > > Fixes: 059f545832be ("spi: add support for microchip "soft" spi controller")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > ---
> > >  drivers/spi/spi-microchip-core-spi.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> > > index b8738190cdcb..e65036cc62f3 100644
> > > --- a/drivers/spi/spi-microchip-core-spi.c
> > > +++ b/drivers/spi/spi-microchip-core-spi.c
> > > @@ -320,6 +320,8 @@ static int mchp_corespi_probe(struct platform_device *pdev)
> > >  	 */
> > >  	ret = of_property_read_string(pdev->dev.of_node, "microchip,protocol-configuration",
> > >  				      &protocol);
> > > +	if (ret)
> > > +		return ret;
> > >  	if (strcmp(protocol, "motorola") != 0)
> > >  		return dev_err_probe(&pdev->dev, -EINVAL,
> > >  				     "CoreSPI: protocol '%s' not supported by this driver\n",
> > 
> > This should probably also complain about not being able to get the
> > property, otherwise nobody is going to be able to figure out what's
> > wrong if we actually hit the error case.
> 
> The one thing to be careful of is that the property has a default, so
> EINVAL needs to be treated differently, so the decision tree is
> something like:
> if (ret == _EINVAL)
> 	<do nothing>
> else if (ret)
> 	abort complaining about malformed
> else if (!motorola)
> 	abort complaining about unsupported mode
> else
> 	<do nothing>
> 
> obviously that can just become two clauses, but you get the idea.

Sure.  I've sent a v2 which defaults to motorola.

regards,
dan carpenter