[053/190] Revert "ecryptfs: replace BUG_ON with error handling code"
diff mbox series

Message ID 20210421130105.1226686-54-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:58 p.m. UTC
This reverts commit 2c2a7552dd6465e8fde6bc9cccf8d66ed1c1eb72.

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: Aditya Pakki <pakki001@umn.edu>
Cc: Tyler Hicks <code@tyhicks.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 fs/ecryptfs/crypto.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Al Viro April 21, 2021, 4:04 p.m. UTC | #1
On Wed, Apr 21, 2021 at 02:58:48PM +0200, Greg Kroah-Hartman wrote:
> This reverts commit 2c2a7552dd6465e8fde6bc9cccf8d66ed1c1eb72.
> 
> 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.

FWIW, commit message on the original (
ecryptfs: replace BUG_ON with error handling code

In crypt_scatterlist, if the crypt_stat argument is not set up
correctly, the kernel crashes. Instead, by returning an error code
upstream, the error is handled safely.

The issue is detected via a static analysis tool written by us.

Fixes: 237fead619984 (ecryptfs: fs/Makefile and fs/Kconfig)
Signed-off-by: Aditya Pakki <pakki001@umn.edu>
Signed-off-by: Tyler Hicks <code@tyhicks.com>
)
really stinks.  First, the analysis: condition being tested is
(!crypt_stat || !crypt_stat->tfm
               || !(crypt_stat->flags & ECRYPTFS_STRUCT_INITIALIZED))
and their patch replaces BUG_ON() with return of -EINVAL.  So the
only thing their tool had detected the presence of BUG_ON().
Was it grep, by any chance?  

IOW, the commit message is "we'd found BUG_ON(); let's replace it
with returning some error value and hope everything works.  Whaddya
mean, how do we know?  Our tool [git grep BUG_ON, that is] says
it's there and look, it *is* there, so if it's ever reached there'll
be trouble.  What, assertion that returning an error will be handled
safely?   'Cuz we saiz so, that's why"


It *is* functionally harmless, AFAICS, but only because the condition
is really impossible.  However,
	* it refers to vague (s)tool they'd produced, nevermind that
all they really do is "find BUG_ON(), replace with returning an error".
	* unlike BUG_ON(), the replacement does *NOT* document the
fact that condition should be impossible.
IMO either should be sufficient for rejecting the patch.
Tyler Hicks April 21, 2021, 4:13 p.m. UTC | #2
On 2021-04-21 16:04:02, Al Viro wrote:
> On Wed, Apr 21, 2021 at 02:58:48PM +0200, Greg Kroah-Hartman wrote:
> > This reverts commit 2c2a7552dd6465e8fde6bc9cccf8d66ed1c1eb72.
> > 
> > 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.
> 
> FWIW, commit message on the original (
> ecryptfs: replace BUG_ON with error handling code
> 
> In crypt_scatterlist, if the crypt_stat argument is not set up
> correctly, the kernel crashes. Instead, by returning an error code
> upstream, the error is handled safely.
> 
> The issue is detected via a static analysis tool written by us.
> 
> Fixes: 237fead619984 (ecryptfs: fs/Makefile and fs/Kconfig)
> Signed-off-by: Aditya Pakki <pakki001@umn.edu>
> Signed-off-by: Tyler Hicks <code@tyhicks.com>
> )
> really stinks.  First, the analysis: condition being tested is
> (!crypt_stat || !crypt_stat->tfm
>                || !(crypt_stat->flags & ECRYPTFS_STRUCT_INITIALIZED))
> and their patch replaces BUG_ON() with return of -EINVAL.  So the
> only thing their tool had detected the presence of BUG_ON().
> Was it grep, by any chance?  
> 
> IOW, the commit message is "we'd found BUG_ON(); let's replace it
> with returning some error value and hope everything works.  Whaddya
> mean, how do we know?  Our tool [git grep BUG_ON, that is] says
> it's there and look, it *is* there, so if it's ever reached there'll
> be trouble.  What, assertion that returning an error will be handled
> safely?   'Cuz we saiz so, that's why"
> 
> 
> It *is* functionally harmless, AFAICS, but only because the condition
> is really impossible.  However,
> 	* it refers to vague (s)tool they'd produced, nevermind that
> all they really do is "find BUG_ON(), replace with returning an error".
> 	* unlike BUG_ON(), the replacement does *NOT* document the
> fact that condition should be impossible.
> IMO either should be sufficient for rejecting the patch.

I agree that it was not a malicious change. There are other places
within the same function that return -EINVAL and the expectation is that
errors from this function should be handled safely.

That said, I can find no real-world reports of this BUG_ON() ever being
a problem and I don't think that there's any actual need for this
change. So, I'm alright with it being reverted considering the
circumstances.

 Acked-by: Tyler Hicks <code@tyhicks.com>

Tyler
Al Viro April 21, 2021, 5:03 p.m. UTC | #3
On Wed, Apr 21, 2021 at 11:13:29AM -0500, Tyler Hicks wrote:

> > It *is* functionally harmless, AFAICS, but only because the condition
> > is really impossible.  However,
> > 	* it refers to vague (s)tool they'd produced, nevermind that
> > all they really do is "find BUG_ON(), replace with returning an error".
> > 	* unlike BUG_ON(), the replacement does *NOT* document the
> > fact that condition should be impossible.
> > IMO either should be sufficient for rejecting the patch.
> 
> I agree that it was not a malicious change. There are other places
> within the same function that return -EINVAL and the expectation is that
> errors from this function should be handled safely.

Umm...  Assuming that failure exits in the callers will function properly
if those conditions are true.  Which is not obvious at all.

> That said, I can find no real-world reports of this BUG_ON() ever being
> a problem and I don't think that there's any actual need for this
> change. So, I'm alright with it being reverted considering the
> circumstances.

AFAICS, at least some parts of that BUG_ON() are provably impossible
(e.g. NULL crypt_stat would've oopsed well upstream of the only call
of that function).  ECRYPTFS_STRUCT_INITIALIZED is set after
ecryptfs_alloc_inode() and never cleared, i.e. it should be present
in ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat.flags for
all inodes.  And crypt_stat we are passing to that thing is
calculated as &(ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat),
which is another reason why it can't be NULL.

Incidentally, what's ecryptfs_setattr() doing with similar check?
It had been introduced in e10f281bca03 "eCryptfs: initialize crypt_stat
in setattr", which claims
    Recent changes in eCryptfs have made it possible to get to ecryptfs_setattr()
    with an uninitialized crypt_stat struct.  This results in a wide and colorful
    variety of unpleasantries.  This patch properly initializes the crypt_stat
    structure in ecryptfs_setattr() when it is necessary to do so.
and AFAICS at that point the call of ecryptfs_init_crypt_stat() in
ecryptfs_alloc_inode() had already been there and EXCRYPTFS_STRUCT_INITIALIZED
had been (unconditionally) set by it.  So how could that check trigger in
ecryptfs_setattr()?  No direct calls of that function (then as well as now),
it's only reachable as ecryptfs_{symlink,dir,main}_iops.setattr.  The first
two could only end up set by ecryptfs_interpose(), for inode returned by
iget5_locked() (i.e. one that had been returned by ->alloc_inode()),
the last is set by ecryptfs_init_inode(), called by ecryptfs_inode_set(), 
passed as callback to iget5_locked() by the same ecryptfs_interpose().
IOW, again, the inode must have been returned by ->alloc_inode().

I realize that it had been a long time ago, but... could somebody
recall what that patch had been about?  Michael?

Commit in question contains another (and much bigger) chunk; do
the comments in commit message refer to it?  Because it really
looks like
	if (!(crypt_stat->flags & ECRYPTFS_STRUCT_INITIALIZED))
		ecryptfs_init_crypt_stat(crypt_stat);
part in ecryptfs_setattr() is a confusing no-op...
Tyler Hicks April 21, 2021, 11:32 p.m. UTC | #4
On 2021-04-21 17:03:24, Al Viro wrote:
> On Wed, Apr 21, 2021 at 11:13:29AM -0500, Tyler Hicks wrote:
> 
> > > It *is* functionally harmless, AFAICS, but only because the condition
> > > is really impossible.  However,
> > > 	* it refers to vague (s)tool they'd produced, nevermind that
> > > all they really do is "find BUG_ON(), replace with returning an error".
> > > 	* unlike BUG_ON(), the replacement does *NOT* document the
> > > fact that condition should be impossible.
> > > IMO either should be sufficient for rejecting the patch.
> > 
> > I agree that it was not a malicious change. There are other places
> > within the same function that return -EINVAL and the expectation is that
> > errors from this function should be handled safely.
> 
> Umm...  Assuming that failure exits in the callers will function properly
> if those conditions are true.  Which is not obvious at all.
> 
> > That said, I can find no real-world reports of this BUG_ON() ever being
> > a problem and I don't think that there's any actual need for this
> > change. So, I'm alright with it being reverted considering the
> > circumstances.
> 
> AFAICS, at least some parts of that BUG_ON() are provably impossible
> (e.g. NULL crypt_stat would've oopsed well upstream of the only call
> of that function).  ECRYPTFS_STRUCT_INITIALIZED is set after
> ecryptfs_alloc_inode() and never cleared, i.e. it should be present
> in ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat.flags for
> all inodes.  And crypt_stat we are passing to that thing is
> calculated as &(ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat),
> which is another reason why it can't be NULL.

I agree.

> 
> Incidentally, what's ecryptfs_setattr() doing with similar check?
> It had been introduced in e10f281bca03 "eCryptfs: initialize crypt_stat
> in setattr", which claims
>     Recent changes in eCryptfs have made it possible to get to ecryptfs_setattr()
>     with an uninitialized crypt_stat struct.  This results in a wide and colorful
>     variety of unpleasantries.  This patch properly initializes the crypt_stat
>     structure in ecryptfs_setattr() when it is necessary to do so.
> and AFAICS at that point the call of ecryptfs_init_crypt_stat() in
> ecryptfs_alloc_inode() had already been there and EXCRYPTFS_STRUCT_INITIALIZED
> had been (unconditionally) set by it.  So how could that check trigger in
> ecryptfs_setattr()?  No direct calls of that function (then as well as now),
> it's only reachable as ecryptfs_{symlink,dir,main}_iops.setattr.  The first
> two could only end up set by ecryptfs_interpose(), for inode returned by
> iget5_locked() (i.e. one that had been returned by ->alloc_inode()),
> the last is set by ecryptfs_init_inode(), called by ecryptfs_inode_set(), 
> passed as callback to iget5_locked() by the same ecryptfs_interpose().
> IOW, again, the inode must have been returned by ->alloc_inode().
> 
> I realize that it had been a long time ago, but... could somebody
> recall what that patch had been about?  Michael?

I looked through the commits that proceeded e10f281bca03 and the only
thing I can think of is the addition of "passthrough" mode where the
lower, encrypted data can be directly read from the eCryptfs mount. It
was introduced in commit e77a56ddceee ("[PATCH] eCryptfs: Encrypted
passthrough"), several months before e10f281bca03. However, I don't see
how it would have left us with an uninitialized crypt_stat in setattr.

Tyler

> Commit in question contains another (and much bigger) chunk; do
> the comments in commit message refer to it?  Because it really
> looks like
> 	if (!(crypt_stat->flags & ECRYPTFS_STRUCT_INITIALIZED))
> 		ecryptfs_init_crypt_stat(crypt_stat);
> part in ecryptfs_setattr() is a confusing no-op...
Greg Kroah-Hartman April 27, 2021, 2:58 p.m. UTC | #5
On Wed, Apr 21, 2021 at 11:13:29AM -0500, Tyler Hicks wrote:
> On 2021-04-21 16:04:02, Al Viro wrote:
> > On Wed, Apr 21, 2021 at 02:58:48PM +0200, Greg Kroah-Hartman wrote:
> > > This reverts commit 2c2a7552dd6465e8fde6bc9cccf8d66ed1c1eb72.
> > > 
> > > 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.
> > 
> > FWIW, commit message on the original (
> > ecryptfs: replace BUG_ON with error handling code
> > 
> > In crypt_scatterlist, if the crypt_stat argument is not set up
> > correctly, the kernel crashes. Instead, by returning an error code
> > upstream, the error is handled safely.
> > 
> > The issue is detected via a static analysis tool written by us.
> > 
> > Fixes: 237fead619984 (ecryptfs: fs/Makefile and fs/Kconfig)
> > Signed-off-by: Aditya Pakki <pakki001@umn.edu>
> > Signed-off-by: Tyler Hicks <code@tyhicks.com>
> > )
> > really stinks.  First, the analysis: condition being tested is
> > (!crypt_stat || !crypt_stat->tfm
> >                || !(crypt_stat->flags & ECRYPTFS_STRUCT_INITIALIZED))
> > and their patch replaces BUG_ON() with return of -EINVAL.  So the
> > only thing their tool had detected the presence of BUG_ON().
> > Was it grep, by any chance?  
> > 
> > IOW, the commit message is "we'd found BUG_ON(); let's replace it
> > with returning some error value and hope everything works.  Whaddya
> > mean, how do we know?  Our tool [git grep BUG_ON, that is] says
> > it's there and look, it *is* there, so if it's ever reached there'll
> > be trouble.  What, assertion that returning an error will be handled
> > safely?   'Cuz we saiz so, that's why"
> > 
> > 
> > It *is* functionally harmless, AFAICS, but only because the condition
> > is really impossible.  However,
> > 	* it refers to vague (s)tool they'd produced, nevermind that
> > all they really do is "find BUG_ON(), replace with returning an error".
> > 	* unlike BUG_ON(), the replacement does *NOT* document the
> > fact that condition should be impossible.
> > IMO either should be sufficient for rejecting the patch.
> 
> I agree that it was not a malicious change. There are other places
> within the same function that return -EINVAL and the expectation is that
> errors from this function should be handled safely.
> 
> That said, I can find no real-world reports of this BUG_ON() ever being
> a problem and I don't think that there's any actual need for this
> change. So, I'm alright with it being reverted considering the
> circumstances.
> 
>  Acked-by: Tyler Hicks <code@tyhicks.com>

Thanks for the review, I've update the commit log message and added your
ack here.

greg k-h

Patch
diff mbox series

diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index 943e523f4c9d..3d8623139538 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -296,10 +296,8 @@  static int crypt_scatterlist(struct ecryptfs_crypt_stat *crypt_stat,
 	struct extent_crypt_result ecr;
 	int rc = 0;
 
-	if (!crypt_stat || !crypt_stat->tfm
-	       || !(crypt_stat->flags & ECRYPTFS_STRUCT_INITIALIZED))
-		return -EINVAL;
-
+	BUG_ON(!crypt_stat || !crypt_stat->tfm
+	       || !(crypt_stat->flags & ECRYPTFS_STRUCT_INITIALIZED));
 	if (unlikely(ecryptfs_verbosity > 0)) {
 		ecryptfs_printk(KERN_DEBUG, "Key size [%zd]; key:\n",
 				crypt_stat->key_size);