diff mbox series

mmc: core: Fix null pointer dereference in bus_shutdown

Message ID 20240119073247.7441-1-sh043.lee@samsung.com
State New
Headers
Series mmc: core: Fix null pointer dereference in bus_shutdown |

Commit Message

이승희 Jan. 19, 2024, 7:32 a.m. UTC 
  When shutting down removable device,
it can be occurred null pointer dereference.

To prevent null pointer dereference,
At first, check null pointer.
Next, block rescan worker to scan removable device during shutdown.

Signed-off-by: Seunghui Lee <sh043.lee@samsung.com>
---
 drivers/mmc/core/bus.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Greg KH Jan. 19, 2024, 8:20 a.m. UTC | #1 
On Fri, Jan 19, 2024 at 04:32:47PM +0900, Seunghui Lee wrote:
> When shutting down removable device,
> it can be occurred null pointer dereference.

How?

And please wrap your lines properly.

> To prevent null pointer dereference,
> At first, check null pointer.
> Next, block rescan worker to scan removable device during shutdown.

Why do two things?

> 
> Signed-off-by: Seunghui Lee <sh043.lee@samsung.com>
> ---
>  drivers/mmc/core/bus.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 0af96548e7da..4f370a6577aa 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -143,9 +143,17 @@ static void mmc_bus_shutdown(struct device *dev)
>  {
>  	struct mmc_driver *drv = to_mmc_driver(dev->driver);
>  	struct mmc_card *card = mmc_dev_to_card(dev);
> -	struct mmc_host *host = card->host;
> +	struct mmc_host *host;
>  	int ret;
>  
> +	if (!drv || !card) {
> +		pr_debug("%s: drv or card is NULL.\n", dev_name(dev));

What is this going to help with?  And why not use dev_dbg()?

How can drv ever be NULL?  That looks impossible to me based on just the
code shown here.

> +		return;
> +	}
> +
> +	host = card->host;

Why is this change needed?  This line can go back to the top just fine,
right?

> +	host->rescan_disable = 1;

Shouldn't this be a separate change?  And what happens if the check for
this is right before you set it?  Where is the locking to prevent the
issue you are attempting to solve?

thanks,

greg k-h
이승희 Jan. 26, 2024, 5:16 a.m. UTC | #2 
> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Friday, January 19, 2024 5:21 PM
> To: Seunghui Lee <sh043.lee@samsung.com>
> Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org;
> ulf.hansson@linaro.org; avri.altman@wdc.com; grant.jung@samsung.com;
> jt77.jang@samsung.com; dh0421.hwang@samsung.com; junwoo80.lee@samsung.com;
> jangsub.yi@samsung.com; cw9316.lee@samsung.com; sh8267.baek@samsung.com;
> wkon.kim@samsung.com
> Subject: Re: [PATCH] mmc: core: Fix null pointer dereference in
> bus_shutdown
> 
> On Fri, Jan 19, 2024 at 04:32:47PM +0900, Seunghui Lee wrote:
> > When shutting down removable device,
> > it can be occurred null pointer dereference.
> 
> How?
> 
> And please wrap your lines properly.
> 
> > To prevent null pointer dereference,
> > At first, check null pointer.
> > Next, block rescan worker to scan removable device during shutdown.
> 
> Why do two things?
> 
> >
> > Signed-off-by: Seunghui Lee <sh043.lee@samsung.com>
> > ---
> >  drivers/mmc/core/bus.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c index
> > 0af96548e7da..4f370a6577aa 100644
> > --- a/drivers/mmc/core/bus.c
> > +++ b/drivers/mmc/core/bus.c
> > @@ -143,9 +143,17 @@ static void mmc_bus_shutdown(struct device *dev)
> > {
> >  	struct mmc_driver *drv = to_mmc_driver(dev->driver);
> >  	struct mmc_card *card = mmc_dev_to_card(dev);
> > -	struct mmc_host *host = card->host;
> > +	struct mmc_host *host;
> >  	int ret;
> >
> > +	if (!drv || !card) {
> > +		pr_debug("%s: drv or card is NULL.\n", dev_name(dev));
> 
> What is this going to help with?  And why not use dev_dbg()?
> 
> How can drv ever be NULL?  That looks impossible to me based on just the
> code shown here.
> 
> > +		return;
> > +	}
> > +
> > +	host = card->host;
> 
> Why is this change needed?  This line can go back to the top just fine,
> right?
> 
> > +	host->rescan_disable = 1;
> 
> Shouldn't this be a separate change?  And what happens if the check for
> this is right before you set it?  Where is the locking to prevent the
> issue you are attempting to solve?
> 
> thanks,
> 
> greg k-h

I've checked the issue again.
This patch is not the proper solution.
I'll reject this patch.
Hi, Thank you for your comment.
diff mbox series

Patch

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 0af96548e7da..4f370a6577aa 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -143,9 +143,17 @@  static void mmc_bus_shutdown(struct device *dev)
 {
 	struct mmc_driver *drv = to_mmc_driver(dev->driver);
 	struct mmc_card *card = mmc_dev_to_card(dev);
-	struct mmc_host *host = card->host;
+	struct mmc_host *host;
 	int ret;
 
+	if (!drv || !card) {
+		pr_debug("%s: drv or card is NULL.\n", dev_name(dev));
+		return;
+	}
+
+	host = card->host;
+	host->rescan_disable = 1;
+
 	if (dev->driver && drv->shutdown)
 		drv->shutdown(card);