[088/190] Revert "mmc_spi: add a status check for spi_sync_locked"
diff mbox series

Message ID 20210421130105.1226686-89-gregkh@linuxfoundation.org
State New, archived
Headers show
Series
  • Revertion of all of the umn.edu commits
Related show

Commit Message

Greg Kroah-Hartman April 21, 2021, 12:59 p.m. UTC
This reverts commit 611025983b7976df0183390a63a2166411d177f1.

Commits from @umn.edu addresses have been found to be submitted in "bad
faith" to try to test the kernel community's ability to review "known
malicious" changes.  The result of these submissions can be found in a
paper published at the 42nd IEEE Symposium on Security and Privacy
entitled, "Open Source Insecurity: Stealthily Introducing
Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University
of Minnesota) and Kangjie Lu (University of Minnesota).

Because of this, all submissions from this group must be reverted from
the kernel tree and will need to be re-reviewed again to determine if
they actually are a valid fix.  Until that work is complete, remove this
change to ensure that no problems are being introduced into the
codebase.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/mmc/host/mmc_spi.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Laurent Pinchart April 21, 2021, 1:19 p.m. UTC | #1
Hi Greg,

Thank you for the patch.

On Wed, Apr 21, 2021 at 02:59:23PM +0200, Greg Kroah-Hartman wrote:
> This reverts commit 611025983b7976df0183390a63a2166411d177f1.
> 
> Commits from @umn.edu addresses have been found to be submitted in "bad
> faith" to try to test the kernel community's ability to review "known
> malicious" changes.  The result of these submissions can be found in a
> paper published at the 42nd IEEE Symposium on Security and Privacy
> entitled, "Open Source Insecurity: Stealthily Introducing
> Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University
> of Minnesota) and Kangjie Lu (University of Minnesota).
> 
> Because of this, all submissions from this group must be reverted from
> the kernel tree and will need to be re-reviewed again to determine if
> they actually are a valid fix.  Until that work is complete, remove this
> change to ensure that no problems are being introduced into the
> codebase.
> 
> Cc: Kangjie Lu <kjlu@umn.edu>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I don't spot an obvious issue with the original patch though.

> ---
>  drivers/mmc/host/mmc_spi.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> index 02f4fd26e76a..cc40b050e302 100644
> --- a/drivers/mmc/host/mmc_spi.c
> +++ b/drivers/mmc/host/mmc_spi.c
> @@ -800,10 +800,6 @@ mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t,
>  	}
>  
>  	status = spi_sync_locked(spi, &host->m);
> -	if (status < 0) {
> -		dev_dbg(&spi->dev, "read error %d\n", status);
> -		return status;
> -	}
>  
>  	if (host->dma_dev) {
>  		dma_sync_single_for_cpu(host->dma_dev,
Ulf Hansson April 22, 2021, 8:08 a.m. UTC | #2
On Wed, 21 Apr 2021 at 15:19, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Greg,
>
> Thank you for the patch.
>
> On Wed, Apr 21, 2021 at 02:59:23PM +0200, Greg Kroah-Hartman wrote:
> > This reverts commit 611025983b7976df0183390a63a2166411d177f1.
> >
> > Commits from @umn.edu addresses have been found to be submitted in "bad
> > faith" to try to test the kernel community's ability to review "known
> > malicious" changes.  The result of these submissions can be found in a
> > paper published at the 42nd IEEE Symposium on Security and Privacy
> > entitled, "Open Source Insecurity: Stealthily Introducing
> > Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University
> > of Minnesota) and Kangjie Lu (University of Minnesota).
> >
> > Because of this, all submissions from this group must be reverted from
> > the kernel tree and will need to be re-reviewed again to determine if
> > they actually are a valid fix.  Until that work is complete, remove this
> > change to ensure that no problems are being introduced into the
> > codebase.
> >
> > Cc: Kangjie Lu <kjlu@umn.edu>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I don't spot an obvious issue with the original patch though.
>
> > ---
> >  drivers/mmc/host/mmc_spi.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> > index 02f4fd26e76a..cc40b050e302 100644
> > --- a/drivers/mmc/host/mmc_spi.c
> > +++ b/drivers/mmc/host/mmc_spi.c
> > @@ -800,10 +800,6 @@ mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t,
> >       }
> >
> >       status = spi_sync_locked(spi, &host->m);
> > -     if (status < 0) {
> > -             dev_dbg(&spi->dev, "read error %d\n", status);
> > -             return status;
> > -     }

Returning here means we never give back the ownership of the buffer to
the CPU. Can that be considered as vulnerability?

If that is that a problem, I can point out that there is already one
more case in this file, where this pattern is repeated. See
mmc_spi_writeblock(). This code has been there since 2007.

> >
> >       if (host->dma_dev) {
> >               dma_sync_single_for_cpu(host->dma_dev,
>
> --
> Regards,
>
> Laurent Pinchart

Kind regards
Uffe
Greg Kroah-Hartman April 28, 2021, 7:18 a.m. UTC | #3
On Thu, Apr 22, 2021 at 10:08:45AM +0200, Ulf Hansson wrote:
> On Wed, 21 Apr 2021 at 15:19, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Greg,
> >
> > Thank you for the patch.
> >
> > On Wed, Apr 21, 2021 at 02:59:23PM +0200, Greg Kroah-Hartman wrote:
> > > This reverts commit 611025983b7976df0183390a63a2166411d177f1.
> > >
> > > Commits from @umn.edu addresses have been found to be submitted in "bad
> > > faith" to try to test the kernel community's ability to review "known
> > > malicious" changes.  The result of these submissions can be found in a
> > > paper published at the 42nd IEEE Symposium on Security and Privacy
> > > entitled, "Open Source Insecurity: Stealthily Introducing
> > > Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University
> > > of Minnesota) and Kangjie Lu (University of Minnesota).
> > >
> > > Because of this, all submissions from this group must be reverted from
> > > the kernel tree and will need to be re-reviewed again to determine if
> > > they actually are a valid fix.  Until that work is complete, remove this
> > > change to ensure that no problems are being introduced into the
> > > codebase.
> > >
> > > Cc: Kangjie Lu <kjlu@umn.edu>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > I don't spot an obvious issue with the original patch though.
> >
> > > ---
> > >  drivers/mmc/host/mmc_spi.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> > > index 02f4fd26e76a..cc40b050e302 100644
> > > --- a/drivers/mmc/host/mmc_spi.c
> > > +++ b/drivers/mmc/host/mmc_spi.c
> > > @@ -800,10 +800,6 @@ mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t,
> > >       }
> > >
> > >       status = spi_sync_locked(spi, &host->m);
> > > -     if (status < 0) {
> > > -             dev_dbg(&spi->dev, "read error %d\n", status);
> > > -             return status;
> > > -     }
> 
> Returning here means we never give back the ownership of the buffer to
> the CPU. Can that be considered as vulnerability?

It's a "resource leak", which is a bug.  If you want to declare that as
a "vulnerability" or not, I do not know.  Personally I do not think it
is...

> If that is that a problem, I can point out that there is already one
> more case in this file, where this pattern is repeated. See
> mmc_spi_writeblock(). This code has been there since 2007.

Yeah, these error paths are impossible to hit anyway.

I'll go drop this patch as it is not correct and will create a "correct"
patch for this as well.

thanks,

greg k-h
Greg Kroah-Hartman April 28, 2021, 7:18 a.m. UTC | #4
On Wed, Apr 21, 2021 at 04:19:07PM +0300, Laurent Pinchart wrote:
> Hi Greg,
> 
> Thank you for the patch.
> 
> On Wed, Apr 21, 2021 at 02:59:23PM +0200, Greg Kroah-Hartman wrote:
> > This reverts commit 611025983b7976df0183390a63a2166411d177f1.
> > 
> > Commits from @umn.edu addresses have been found to be submitted in "bad
> > faith" to try to test the kernel community's ability to review "known
> > malicious" changes.  The result of these submissions can be found in a
> > paper published at the 42nd IEEE Symposium on Security and Privacy
> > entitled, "Open Source Insecurity: Stealthily Introducing
> > Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University
> > of Minnesota) and Kangjie Lu (University of Minnesota).
> > 
> > Because of this, all submissions from this group must be reverted from
> > the kernel tree and will need to be re-reviewed again to determine if
> > they actually are a valid fix.  Until that work is complete, remove this
> > change to ensure that no problems are being introduced into the
> > codebase.
> > 
> > Cc: Kangjie Lu <kjlu@umn.edu>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks for the review!

> I don't spot an obvious issue with the original patch though.

Ulf did :)
Laurent Pinchart April 28, 2021, 8:41 a.m. UTC | #5
Hi Greg,

On Wed, Apr 28, 2021 at 09:18:03AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 22, 2021 at 10:08:45AM +0200, Ulf Hansson wrote:
> > On Wed, 21 Apr 2021 at 15:19, Laurent Pinchart wrote:
> > > On Wed, Apr 21, 2021 at 02:59:23PM +0200, Greg Kroah-Hartman wrote:
> > > > This reverts commit 611025983b7976df0183390a63a2166411d177f1.
> > > >
> > > > Commits from @umn.edu addresses have been found to be submitted in "bad
> > > > faith" to try to test the kernel community's ability to review "known
> > > > malicious" changes.  The result of these submissions can be found in a
> > > > paper published at the 42nd IEEE Symposium on Security and Privacy
> > > > entitled, "Open Source Insecurity: Stealthily Introducing
> > > > Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University
> > > > of Minnesota) and Kangjie Lu (University of Minnesota).
> > > >
> > > > Because of this, all submissions from this group must be reverted from
> > > > the kernel tree and will need to be re-reviewed again to determine if
> > > > they actually are a valid fix.  Until that work is complete, remove this
> > > > change to ensure that no problems are being introduced into the
> > > > codebase.
> > > >
> > > > Cc: Kangjie Lu <kjlu@umn.edu>
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >
> > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > I don't spot an obvious issue with the original patch though.
> > >
> > > > ---
> > > >  drivers/mmc/host/mmc_spi.c | 4 ----
> > > >  1 file changed, 4 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> > > > index 02f4fd26e76a..cc40b050e302 100644
> > > > --- a/drivers/mmc/host/mmc_spi.c
> > > > +++ b/drivers/mmc/host/mmc_spi.c
> > > > @@ -800,10 +800,6 @@ mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t,
> > > >       }
> > > >
> > > >       status = spi_sync_locked(spi, &host->m);
> > > > -     if (status < 0) {
> > > > -             dev_dbg(&spi->dev, "read error %d\n", status);
> > > > -             return status;
> > > > -     }
> > 
> > Returning here means we never give back the ownership of the buffer to
> > the CPU. Can that be considered as vulnerability?
> 
> It's a "resource leak", which is a bug.  If you want to declare that as
> a "vulnerability" or not, I do not know.  Personally I do not think it
> is...

How is that a resource leak ? The dma_sync_single_for_device() calls
above this block don't take the buffer ownership away from the CPU in a
way that leaks it.

> > If that is that a problem, I can point out that there is already one
> > more case in this file, where this pattern is repeated. See
> > mmc_spi_writeblock(). This code has been there since 2007.
> 
> Yeah, these error paths are impossible to hit anyway.
> 
> I'll go drop this patch as it is not correct and will create a "correct"
> patch for this as well.
Laurent Pinchart April 28, 2021, 8:42 a.m. UTC | #6
Hi Greg,

On Wed, Apr 28, 2021 at 09:18:18AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 21, 2021 at 04:19:07PM +0300, Laurent Pinchart wrote:
> > On Wed, Apr 21, 2021 at 02:59:23PM +0200, Greg Kroah-Hartman wrote:
> > > This reverts commit 611025983b7976df0183390a63a2166411d177f1.
> > > 
> > > Commits from @umn.edu addresses have been found to be submitted in "bad
> > > faith" to try to test the kernel community's ability to review "known
> > > malicious" changes.  The result of these submissions can be found in a
> > > paper published at the 42nd IEEE Symposium on Security and Privacy
> > > entitled, "Open Source Insecurity: Stealthily Introducing
> > > Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University
> > > of Minnesota) and Kangjie Lu (University of Minnesota).
> > > 
> > > Because of this, all submissions from this group must be reverted from
> > > the kernel tree and will need to be re-reviewed again to determine if
> > > they actually are a valid fix.  Until that work is complete, remove this
> > > change to ensure that no problems are being introduced into the
> > > codebase.
> > > 
> > > Cc: Kangjie Lu <kjlu@umn.edu>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks for the review!
> 
> > I don't spot an obvious issue with the original patch though.
> 
> Ulf did :)

I've replied separately, I still think it's not an issue :-)
Greg Kroah-Hartman April 29, 2021, 2:25 p.m. UTC | #7
On Wed, Apr 28, 2021 at 11:41:25AM +0300, Laurent Pinchart wrote:
> Hi Greg,
> 
> On Wed, Apr 28, 2021 at 09:18:03AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Apr 22, 2021 at 10:08:45AM +0200, Ulf Hansson wrote:
> > > On Wed, 21 Apr 2021 at 15:19, Laurent Pinchart wrote:
> > > > On Wed, Apr 21, 2021 at 02:59:23PM +0200, Greg Kroah-Hartman wrote:
> > > > > This reverts commit 611025983b7976df0183390a63a2166411d177f1.
> > > > >
> > > > > Commits from @umn.edu addresses have been found to be submitted in "bad
> > > > > faith" to try to test the kernel community's ability to review "known
> > > > > malicious" changes.  The result of these submissions can be found in a
> > > > > paper published at the 42nd IEEE Symposium on Security and Privacy
> > > > > entitled, "Open Source Insecurity: Stealthily Introducing
> > > > > Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University
> > > > > of Minnesota) and Kangjie Lu (University of Minnesota).
> > > > >
> > > > > Because of this, all submissions from this group must be reverted from
> > > > > the kernel tree and will need to be re-reviewed again to determine if
> > > > > they actually are a valid fix.  Until that work is complete, remove this
> > > > > change to ensure that no problems are being introduced into the
> > > > > codebase.
> > > > >
> > > > > Cc: Kangjie Lu <kjlu@umn.edu>
> > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > >
> > > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >
> > > > I don't spot an obvious issue with the original patch though.
> > > >
> > > > > ---
> > > > >  drivers/mmc/host/mmc_spi.c | 4 ----
> > > > >  1 file changed, 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> > > > > index 02f4fd26e76a..cc40b050e302 100644
> > > > > --- a/drivers/mmc/host/mmc_spi.c
> > > > > +++ b/drivers/mmc/host/mmc_spi.c
> > > > > @@ -800,10 +800,6 @@ mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t,
> > > > >       }
> > > > >
> > > > >       status = spi_sync_locked(spi, &host->m);
> > > > > -     if (status < 0) {
> > > > > -             dev_dbg(&spi->dev, "read error %d\n", status);
> > > > > -             return status;
> > > > > -     }
> > > 
> > > Returning here means we never give back the ownership of the buffer to
> > > the CPU. Can that be considered as vulnerability?
> > 
> > It's a "resource leak", which is a bug.  If you want to declare that as
> > a "vulnerability" or not, I do not know.  Personally I do not think it
> > is...
> 
> How is that a resource leak ? The dma_sync_single_for_device() calls
> above this block don't take the buffer ownership away from the CPU in a
> way that leaks it.

Ick, this is really twisty.

The calls to dma_sync_single_for_device() call down to
dma_direct_sync_single_for_device() for when you can directly talk to
the hardware, or to the platform sync_single_for_device() callback for
arches that can not.

Those are messy, but the worst they all seem to do is invalidate or
flush some caches, no type of resource allocation that I could determine
here.

So I'll trust you, and drop the revert and mark this one as "good
patch" :)

Thanks again for the review, much appreciated.

greg k-h

Patch
diff mbox series

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 02f4fd26e76a..cc40b050e302 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -800,10 +800,6 @@  mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t,
 	}
 
 	status = spi_sync_locked(spi, &host->m);
-	if (status < 0) {
-		dev_dbg(&spi->dev, "read error %d\n", status);
-		return status;
-	}
 
 	if (host->dma_dev) {
 		dma_sync_single_for_cpu(host->dma_dev,