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

[Stefan Schmidt]

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.

regards
Stefan Schmidt