Re: [PATCH 084/190] Revert "net: ieee802154: fix missing checks for regmap_update_bits"

[Greg Kroah-Hartman]

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@xxxxxxx>
> > > > Cc: Mukesh Ojha <mojha@xxxxxxxxxxxxxx>
> > > > Cc: Stefan Schmidt <stefan@xxxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >    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@xxxxxxxxxxxxxxxxxx>
> > 
> > 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