diff mbox series

[<PATCH,v1>,1/4] mmc: core: Add check for NULL pointer access

Message ID b328b981a785525b8424b4ab2197dc1ec54417d1.1582839544.git.nguyenb@codeaurora.org
State New, archived
Headers show
Series SD card bug fixes | expand

Commit Message

Bao D. Nguyen Feb. 27, 2020, 10:05 p.m. UTC
If the SD card is removed, the mmc_card pointer can be set to NULL
by the mmc_sd_remove() function. Check mmc_card pointer to avoid NULL
pointer access.

Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/mmc/core/bus.c  | 5 +++++
 drivers/mmc/core/core.c | 3 +++
 2 files changed, 8 insertions(+)

Comments

Ulf Hansson Feb. 28, 2020, 6:46 a.m. UTC | #1
On Thu, 27 Feb 2020 at 23:06, Bao D. Nguyen <nguyenb@codeaurora.org> wrote:
>
> If the SD card is removed, the mmc_card pointer can be set to NULL
> by the mmc_sd_remove() function. Check mmc_card pointer to avoid NULL
> pointer access.
>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/mmc/core/bus.c  | 5 +++++
>  drivers/mmc/core/core.c | 3 +++
>  2 files changed, 8 insertions(+)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 74de3f2..4558f51 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -131,6 +131,11 @@ static void mmc_bus_shutdown(struct device *dev)
>         struct mmc_host *host = card->host;
>         int ret;

This obviously doesn't solve anything as we have already dereferenced
the card->host above. In other words we should hit a NULL pointer
dereference bug then.

More exactly, how do you trigger this problem?

>
> +       if (!card) {
> +               dev_dbg(dev, "%s: %s: card is NULL\n", dev_name(dev), __func__);
> +               return;
> +       }
> +
>         if (dev->driver && drv->shutdown)
>                 drv->shutdown(card);
>

[...]

Kind regards
Uffe
Bao D. Nguyen March 6, 2020, 3:38 a.m. UTC | #2
On 2020-02-27 22:46, Ulf Hansson wrote:
> On Thu, 27 Feb 2020 at 23:06, Bao D. Nguyen <nguyenb@codeaurora.org> 
> wrote:
>> 
>> If the SD card is removed, the mmc_card pointer can be set to NULL
>> by the mmc_sd_remove() function. Check mmc_card pointer to avoid NULL
>> pointer access.
>> 
>> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>> ---
>>  drivers/mmc/core/bus.c  | 5 +++++
>>  drivers/mmc/core/core.c | 3 +++
>>  2 files changed, 8 insertions(+)
>> 
>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
>> index 74de3f2..4558f51 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -131,6 +131,11 @@ static void mmc_bus_shutdown(struct device *dev)
>>         struct mmc_host *host = card->host;
>>         int ret;
> 
> This obviously doesn't solve anything as we have already dereferenced
> the card->host above. In other words we should hit a NULL pointer
> dereference bug then.
> 
> More exactly, how do you trigger this problem?
I am porting this fix in the older kernel version 3.4. In that version 
3.4, the pointer check was needed.
Obviously, this NULL pointer check is not helping anything here as you 
pointed out. I will remove this check and resubmit.

> 
>> 
>> +       if (!card) {
>> +               dev_dbg(dev, "%s: %s: card is NULL\n", dev_name(dev), 
>> __func__);
>> +               return;
>> +       }
>> +
>>         if (dev->driver && drv->shutdown)
>>                 drv->shutdown(card);
>> 
> 
> [...]
> 
> Kind regards
> Uffe
Ulf Hansson March 6, 2020, 10:29 a.m. UTC | #3
On Fri, 6 Mar 2020 at 04:38, <nguyenb@codeaurora.org> wrote:
>
> On 2020-02-27 22:46, Ulf Hansson wrote:
> > On Thu, 27 Feb 2020 at 23:06, Bao D. Nguyen <nguyenb@codeaurora.org>
> > wrote:
> >>
> >> If the SD card is removed, the mmc_card pointer can be set to NULL
> >> by the mmc_sd_remove() function. Check mmc_card pointer to avoid NULL
> >> pointer access.
> >>
> >> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> >> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> >> ---
> >>  drivers/mmc/core/bus.c  | 5 +++++
> >>  drivers/mmc/core/core.c | 3 +++
> >>  2 files changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> >> index 74de3f2..4558f51 100644
> >> --- a/drivers/mmc/core/bus.c
> >> +++ b/drivers/mmc/core/bus.c
> >> @@ -131,6 +131,11 @@ static void mmc_bus_shutdown(struct device *dev)
> >>         struct mmc_host *host = card->host;
> >>         int ret;
> >
> > This obviously doesn't solve anything as we have already dereferenced
> > the card->host above. In other words we should hit a NULL pointer
> > dereference bug then.
> >
> > More exactly, how do you trigger this problem?
> I am porting this fix in the older kernel version 3.4. In that version
> 3.4, the pointer check was needed.
> Obviously, this NULL pointer check is not helping anything here as you
> pointed out. I will remove this check and resubmit.

Don't get me wrong, we have had serious errors in the mmc core before,
that we needed to fix. Perhaps, the series are addressing some issues
like this, I don't know.

The point is, either you need to provide a detailed theoretical proof,
or a description of how to reproduce the problem. If not, I don't see
how I can pick any of your patches, sorry.

[...]

Kind regards
Uffe
diff mbox series

Patch

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 74de3f2..4558f51 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -131,6 +131,11 @@  static void mmc_bus_shutdown(struct device *dev)
 	struct mmc_host *host = card->host;
 	int ret;
 
+	if (!card) {
+		dev_dbg(dev, "%s: %s: card is NULL\n", dev_name(dev), __func__);
+		return;
+	}
+
 	if (dev->driver && drv->shutdown)
 		drv->shutdown(card);
 
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6b38c19..94441a0 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -666,6 +666,9 @@  void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
 {
 	unsigned int mult;
 
+	if (!card)
+		return;
+
 	/*
 	 * SDIO cards only define an upper 1 s limit on access.
 	 */