| Message ID | 20210414151242.102313-1-krzysztof.kozlowski@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] mmc: sdhci-s3c: fix possible NULL pointer dereference when probed via platform | expand |
On 14/04/2021 17:12, Krzysztof Kozlowski wrote: > The driver can be matched by legacy platform way or OF-device matching. > In the first case, of_match_node() can return NULL, which immediately > would be dereferenced to get the match data. > > Addresses-Coverity: Dereference null return value > Fixes: cd1b00eb24b0 ("mmc: sdhci-s3c: Add device tree support") > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > --- > > Not tested on HW, but the code should be equivalent (plus safer). > > drivers/mmc/host/sdhci-s3c.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c > index f48a788a9d3d..8e1dca625620 100644 > --- a/drivers/mmc/host/sdhci-s3c.c > +++ b/drivers/mmc/host/sdhci-s3c.c > @@ -20,6 +20,7 @@ > #include <linux/gpio.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/of_gpio.h> > #include <linux/pm.h> > #include <linux/pm_runtime.h> > @@ -461,19 +462,12 @@ static int sdhci_s3c_parse_dt(struct device *dev, > } > #endif > > -#ifdef CONFIG_OF > -static const struct of_device_id sdhci_s3c_dt_match[]; > -#endif > - > static inline struct sdhci_s3c_drv_data *sdhci_s3c_get_driver_data( > struct platform_device *pdev) > { > #ifdef CONFIG_OF > - if (pdev->dev.of_node) { > - const struct of_device_id *match; > - match = of_match_node(sdhci_s3c_dt_match, pdev->dev.of_node); Now I have second thoughts whether NULL pointer can actually happen. If device is matched via platform/board files, maybe the pdev->dev.of_node will be NULL thus skipping this branch? Could there be a case where device is matched via platform_device_id() (which has different name than compatible!) and (pdev->dev.of_node) is still assigned? Maybe in case of out of tree DTS? Anyway, the patch makes the code simpler/smaller, so I still think it is reasonable. Just the severity of issue is questionable... Best regards, Krzysztof
On 14.04.2021 17:25, Krzysztof Kozlowski wrote: > On 14/04/2021 17:12, Krzysztof Kozlowski wrote: >> The driver can be matched by legacy platform way or OF-device matching. >> In the first case, of_match_node() can return NULL, which immediately >> would be dereferenced to get the match data. >> >> Addresses-Coverity: Dereference null return value >> Fixes: cd1b00eb24b0 ("mmc: sdhci-s3c: Add device tree support") >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >> -#ifdef CONFIG_OF >> -static const struct of_device_id sdhci_s3c_dt_match[]; >> -#endif >> - >> static inline struct sdhci_s3c_drv_data *sdhci_s3c_get_driver_data( >> struct platform_device *pdev) >> { >> #ifdef CONFIG_OF >> - if (pdev->dev.of_node) { >> - const struct of_device_id *match; >> - match = of_match_node(sdhci_s3c_dt_match, pdev->dev.of_node); > > Now I have second thoughts whether NULL pointer can actually happen. If > device is matched via platform/board files, maybe the pdev->dev.of_node > will be NULL thus skipping this branch? > > Could there be a case where device is matched via platform_device_id() > (which has different name than compatible!) and (pdev->dev.of_node) is > still assigned? Maybe in case of out of tree DTS? That seems unlikely, the platform device would need to be initialized via board file and then its of_node assigned somehow. It doesn't make much sense to me. We either use board file or dtb to instantiate devices. > Anyway, the patch makes the code simpler/smaller, so I still think it is > reasonable. Just the severity of issue is questionable... Yes, the patch looks good. Similar cleanups are already done in most of the s3c/s5p/exynos drivers. -- Thanks, Sylwester
On 14/04/2021 18:49, Sylwester Nawrocki wrote: > On 14.04.2021 17:25, Krzysztof Kozlowski wrote: >> On 14/04/2021 17:12, Krzysztof Kozlowski wrote: >>> The driver can be matched by legacy platform way or OF-device matching. >>> In the first case, of_match_node() can return NULL, which immediately >>> would be dereferenced to get the match data. >>> >>> Addresses-Coverity: Dereference null return value >>> Fixes: cd1b00eb24b0 ("mmc: sdhci-s3c: Add device tree support") >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > >>> -#ifdef CONFIG_OF >>> -static const struct of_device_id sdhci_s3c_dt_match[]; >>> -#endif >>> - >>> static inline struct sdhci_s3c_drv_data *sdhci_s3c_get_driver_data( >>> struct platform_device *pdev) >>> { >>> #ifdef CONFIG_OF >>> - if (pdev->dev.of_node) { >>> - const struct of_device_id *match; >>> - match = of_match_node(sdhci_s3c_dt_match, pdev->dev.of_node); >> >> Now I have second thoughts whether NULL pointer can actually happen. If >> device is matched via platform/board files, maybe the pdev->dev.of_node >> will be NULL thus skipping this branch? >> >> Could there be a case where device is matched via platform_device_id() >> (which has different name than compatible!) and (pdev->dev.of_node) is >> still assigned? Maybe in case of out of tree DTS? > > That seems unlikely, the platform device would need to be initialized > via board file and then its of_node assigned somehow. It doesn't make > much sense to me. We either use board file or dtb to instantiate devices. > >> Anyway, the patch makes the code simpler/smaller, so I still think it is >> reasonable. Just the severity of issue is questionable... > > Yes, the patch looks good. Similar cleanups are already done in most of > the s3c/s5p/exynos drivers. Thanks Sylwester, I'll resend with different description. Best regards, Krzysztof
diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c index f48a788a9d3d..8e1dca625620 100644 --- a/drivers/mmc/host/sdhci-s3c.c +++ b/drivers/mmc/host/sdhci-s3c.c @@ -20,6 +20,7 @@ #include <linux/gpio.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/of_gpio.h> #include <linux/pm.h> #include <linux/pm_runtime.h> @@ -461,19 +462,12 @@ static int sdhci_s3c_parse_dt(struct device *dev, } #endif -#ifdef CONFIG_OF -static const struct of_device_id sdhci_s3c_dt_match[]; -#endif - static inline struct sdhci_s3c_drv_data *sdhci_s3c_get_driver_data( struct platform_device *pdev) { #ifdef CONFIG_OF - if (pdev->dev.of_node) { - const struct of_device_id *match; - match = of_match_node(sdhci_s3c_dt_match, pdev->dev.of_node); - return (struct sdhci_s3c_drv_data *)match->data; - } + if (pdev->dev.of_node) + return (struct sdhci_s3c_drv_data *)of_device_get_match_data(&pdev->dev); #endif return (struct sdhci_s3c_drv_data *) platform_get_device_id(pdev)->driver_data;
The driver can be matched by legacy platform way or OF-device matching. In the first case, of_match_node() can return NULL, which immediately would be dereferenced to get the match data. Addresses-Coverity: Dereference null return value Fixes: cd1b00eb24b0 ("mmc: sdhci-s3c: Add device tree support") Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> --- Not tested on HW, but the code should be equivalent (plus safer). drivers/mmc/host/sdhci-s3c.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)