[084/190] Revert "net: ieee802154: fix missing checks for regmap_update_bits"
diff mbox series

Message ID 20210421130105.1226686-85-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 22e8860cf8f777fbf6a83f2fb7127f682a8e9de4.

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: Mukesh Ojha <mojha@codeaurora.org>
Cc: Stefan Schmidt <stefan@datenfreihafen.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/ieee802154/mcr20a.c | 6 ------
 1 file changed, 6 deletions(-)

Comments

Stefan Schmidt April 21, 2021, 1:23 p.m. UTC | #1
Hello.

On 21.04.21 14:59, Greg Kroah-Hartman wrote:
> This reverts commit 22e8860cf8f777fbf6a83f2fb7127f682a8e9de4.
> 
> 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: Mukesh Ojha <mojha@codeaurora.org>
> Cc: Stefan Schmidt <stefan@datenfreihafen.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>   drivers/net/ieee802154/mcr20a.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
> index 8dc04e2590b1..2ce5b41983f8 100644
> --- a/drivers/net/ieee802154/mcr20a.c
> +++ b/drivers/net/ieee802154/mcr20a.c
> @@ -524,8 +524,6 @@ mcr20a_start(struct ieee802154_hw *hw)
>   	dev_dbg(printdev(lp), "no slotted operation\n");
>   	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL1,
>   				 DAR_PHY_CTRL1_SLOTTED, 0x0);
> -	if (ret < 0)
> -		return ret;
>   
>   	/* enable irq */
>   	enable_irq(lp->spi->irq);
> @@ -533,15 +531,11 @@ mcr20a_start(struct ieee802154_hw *hw)
>   	/* Unmask SEQ interrupt */
>   	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL2,
>   				 DAR_PHY_CTRL2_SEQMSK, 0x0);
> -	if (ret < 0)
> -		return ret;
>   
>   	/* Start the RX sequence */
>   	dev_dbg(printdev(lp), "start the RX sequence\n");
>   	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL1,
>   				 DAR_PHY_CTRL1_XCVSEQ_MASK, MCR20A_XCVSEQ_RX);
> -	if (ret < 0)
> -		return ret;
>   
>   	return 0;
>   }
> 


Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>

regards
Stefan Schmidt
Greg Kroah-Hartman April 27, 2021, 1:39 p.m. UTC | #2
On Wed, Apr 21, 2021 at 03:23:06PM +0200, Stefan Schmidt wrote:
> Hello.
> 
> On 21.04.21 14:59, Greg Kroah-Hartman wrote:
> > This reverts commit 22e8860cf8f777fbf6a83f2fb7127f682a8e9de4.
> > 
> > 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: Mukesh Ojha <mojha@codeaurora.org>
> > Cc: Stefan Schmidt <stefan@datenfreihafen.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >   drivers/net/ieee802154/mcr20a.c | 6 ------
> >   1 file changed, 6 deletions(-)
> > 
> > diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
> > index 8dc04e2590b1..2ce5b41983f8 100644
> > --- a/drivers/net/ieee802154/mcr20a.c
> > +++ b/drivers/net/ieee802154/mcr20a.c
> > @@ -524,8 +524,6 @@ mcr20a_start(struct ieee802154_hw *hw)
> >   	dev_dbg(printdev(lp), "no slotted operation\n");
> >   	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL1,
> >   				 DAR_PHY_CTRL1_SLOTTED, 0x0);
> > -	if (ret < 0)
> > -		return ret;
> >   	/* enable irq */
> >   	enable_irq(lp->spi->irq);
> > @@ -533,15 +531,11 @@ mcr20a_start(struct ieee802154_hw *hw)
> >   	/* Unmask SEQ interrupt */
> >   	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL2,
> >   				 DAR_PHY_CTRL2_SEQMSK, 0x0);
> > -	if (ret < 0)
> > -		return ret;
> >   	/* Start the RX sequence */
> >   	dev_dbg(printdev(lp), "start the RX sequence\n");
> >   	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL1,
> >   				 DAR_PHY_CTRL1_XCVSEQ_MASK, MCR20A_XCVSEQ_RX);
> > -	if (ret < 0)
> > -		return ret;
> >   	return 0;
> >   }
> > 
> 
> 
> Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>

Thanks for the review, but in re-reviewing this, I'll drop the revert as
it looks correct to me.

thanks,

greg k-h
Stefan Schmidt April 27, 2021, 1:46 p.m. UTC | #3
Hello Greg.

On 27.04.21 15:39, Greg Kroah-Hartman wrote:
> On Wed, Apr 21, 2021 at 03:23:06PM +0200, Stefan Schmidt wrote:
>> Hello.
>>
>> On 21.04.21 14:59, Greg Kroah-Hartman wrote:
>>> This reverts commit 22e8860cf8f777fbf6a83f2fb7127f682a8e9de4.
>>>
>>> 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: Mukesh Ojha <mojha@codeaurora.org>
>>> Cc: Stefan Schmidt <stefan@datenfreihafen.org>
>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> ---
>>>    drivers/net/ieee802154/mcr20a.c | 6 ------
>>>    1 file changed, 6 deletions(-)
>>>
>>> diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
>>> index 8dc04e2590b1..2ce5b41983f8 100644
>>> --- a/drivers/net/ieee802154/mcr20a.c
>>> +++ b/drivers/net/ieee802154/mcr20a.c
>>> @@ -524,8 +524,6 @@ mcr20a_start(struct ieee802154_hw *hw)
>>>    	dev_dbg(printdev(lp), "no slotted operation\n");
>>>    	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL1,
>>>    				 DAR_PHY_CTRL1_SLOTTED, 0x0);
>>> -	if (ret < 0)
>>> -		return ret;
>>>    	/* enable irq */
>>>    	enable_irq(lp->spi->irq);
>>> @@ -533,15 +531,11 @@ mcr20a_start(struct ieee802154_hw *hw)
>>>    	/* Unmask SEQ interrupt */
>>>    	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL2,
>>>    				 DAR_PHY_CTRL2_SEQMSK, 0x0);
>>> -	if (ret < 0)
>>> -		return ret;
>>>    	/* Start the RX sequence */
>>>    	dev_dbg(printdev(lp), "start the RX sequence\n");
>>>    	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL1,
>>>    				 DAR_PHY_CTRL1_XCVSEQ_MASK, MCR20A_XCVSEQ_RX);
>>> -	if (ret < 0)
>>> -		return ret;
>>>    	return 0;
>>>    }
>>>
>>
>>
>> Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>
> 
> Thanks for the review, but in re-reviewing this, I'll drop the revert as
> it looks correct to me.

It is correct. We missed the return checking when the driver came in 
initially.

My Acked-by was really not about if the reverted patch was a security 
risk, but about the fact that you wanted to sort them out individually 
due to the Hypocrite Commits paper before getting them back in.

If you are happy to change the approach to only revert patches you are 
in doubt (this one is really not one of them) I am happy to keep this 
patch in.

regards
Stefan Schmidt
Greg Kroah-Hartman April 27, 2021, 2:05 p.m. UTC | #4
On Tue, Apr 27, 2021 at 03:46:27PM +0200, Stefan Schmidt wrote:
> Hello Greg.
> 
> On 27.04.21 15:39, Greg Kroah-Hartman wrote:
> > On Wed, Apr 21, 2021 at 03:23:06PM +0200, Stefan Schmidt wrote:
> > > Hello.
> > > 
> > > On 21.04.21 14:59, Greg Kroah-Hartman wrote:
> > > > This reverts commit 22e8860cf8f777fbf6a83f2fb7127f682a8e9de4.
> > > > 
> > > > 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: Mukesh Ojha <mojha@codeaurora.org>
> > > > Cc: Stefan Schmidt <stefan@datenfreihafen.org>
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > ---
> > > >    drivers/net/ieee802154/mcr20a.c | 6 ------
> > > >    1 file changed, 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
> > > > index 8dc04e2590b1..2ce5b41983f8 100644
> > > > --- a/drivers/net/ieee802154/mcr20a.c
> > > > +++ b/drivers/net/ieee802154/mcr20a.c
> > > > @@ -524,8 +524,6 @@ mcr20a_start(struct ieee802154_hw *hw)
> > > >    	dev_dbg(printdev(lp), "no slotted operation\n");
> > > >    	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL1,
> > > >    				 DAR_PHY_CTRL1_SLOTTED, 0x0);
> > > > -	if (ret < 0)
> > > > -		return ret;
> > > >    	/* enable irq */
> > > >    	enable_irq(lp->spi->irq);
> > > > @@ -533,15 +531,11 @@ mcr20a_start(struct ieee802154_hw *hw)
> > > >    	/* Unmask SEQ interrupt */
> > > >    	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL2,
> > > >    				 DAR_PHY_CTRL2_SEQMSK, 0x0);
> > > > -	if (ret < 0)
> > > > -		return ret;
> > > >    	/* Start the RX sequence */
> > > >    	dev_dbg(printdev(lp), "start the RX sequence\n");
> > > >    	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL1,
> > > >    				 DAR_PHY_CTRL1_XCVSEQ_MASK, MCR20A_XCVSEQ_RX);
> > > > -	if (ret < 0)
> > > > -		return ret;
> > > >    	return 0;
> > > >    }
> > > > 
> > > 
> > > 
> > > Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>
> > 
> > Thanks for the review, but in re-reviewing this, I'll drop the revert as
> > it looks correct to me.
> 
> It is correct. We missed the return checking when the driver came in
> initially.
> 
> My Acked-by was really not about if the reverted patch was a security risk,
> but about the fact that you wanted to sort them out individually due to the
> Hypocrite Commits paper before getting them back in.
> 
> If you are happy to change the approach to only revert patches you are in
> doubt (this one is really not one of them) I am happy to keep this patch in.

My first step was "revert them all so we know what to review", and now
I'm going back and reviewing them all (and taking the review comments
from everyone else who was nice enough to help out)"

So it's easy for me to drop this from the series now, thanks!

greg k-h

Patch
diff mbox series

diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
index 8dc04e2590b1..2ce5b41983f8 100644
--- a/drivers/net/ieee802154/mcr20a.c
+++ b/drivers/net/ieee802154/mcr20a.c
@@ -524,8 +524,6 @@  mcr20a_start(struct ieee802154_hw *hw)
 	dev_dbg(printdev(lp), "no slotted operation\n");
 	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL1,
 				 DAR_PHY_CTRL1_SLOTTED, 0x0);
-	if (ret < 0)
-		return ret;
 
 	/* enable irq */
 	enable_irq(lp->spi->irq);
@@ -533,15 +531,11 @@  mcr20a_start(struct ieee802154_hw *hw)
 	/* Unmask SEQ interrupt */
 	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL2,
 				 DAR_PHY_CTRL2_SEQMSK, 0x0);
-	if (ret < 0)
-		return ret;
 
 	/* Start the RX sequence */
 	dev_dbg(printdev(lp), "start the RX sequence\n");
 	ret = regmap_update_bits(lp->regmap_dar, DAR_PHY_CTRL1,
 				 DAR_PHY_CTRL1_XCVSEQ_MASK, MCR20A_XCVSEQ_RX);
-	if (ret < 0)
-		return ret;
 
 	return 0;
 }