2021-04-07 13:43:12

by Aditya Pakki

[permalink] [raw]
Subject: [PATCH] SUNRPC: Add a check for gss_release_msg

In gss_pipe_destroy_msg(), in case of error in msg, gss_release_msg
deletes gss_msg. The patch adds a check to avoid a potential double
free.

Signed-off-by: Aditya Pakki <[email protected]>
---
net/sunrpc/auth_gss/auth_gss.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 5f42aa5fc612..eb52eebb3923 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -848,7 +848,8 @@ gss_pipe_destroy_msg(struct rpc_pipe_msg *msg)
warn_gssd();
gss_release_msg(gss_msg);
}
- gss_release_msg(gss_msg);
+ if (gss_msg)
+ gss_release_msg(gss_msg);
}

static void gss_pipe_dentry_destroy(struct dentry *dir,
--
2.25.1


2021-04-07 21:20:23

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Tue, Apr 06, 2021 at 07:16:56PM -0500, Aditya Pakki wrote:
> In gss_pipe_destroy_msg(), in case of error in msg, gss_release_msg
> deletes gss_msg. The patch adds a check to avoid a potential double
> free.

We're already dereferenced msg. Nothing has set gss_msg to NULL. It's
the gss_msg->count reference count that's supposed to prevent double
frees.

Did you see an actual bug or warning from some tool, and if so, could
you share the details?

--b.

>
> Signed-off-by: Aditya Pakki <[email protected]>
> ---
> net/sunrpc/auth_gss/auth_gss.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> index 5f42aa5fc612..eb52eebb3923 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -848,7 +848,8 @@ gss_pipe_destroy_msg(struct rpc_pipe_msg *msg)
> warn_gssd();
> gss_release_msg(gss_msg);
> }
> - gss_release_msg(gss_msg);
> + if (gss_msg)
> + gss_release_msg(gss_msg);
> }
>
> static void gss_pipe_dentry_destroy(struct dentry *dir,
> --
> 2.25.1

2021-04-08 15:01:49

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Tue, 2021-04-06 at 19:16 -0500, Aditya Pakki wrote:
> In gss_pipe_destroy_msg(), in case of error in msg, gss_release_msg
> deletes gss_msg. The patch adds a check to avoid a potential double
> free.
>
> Signed-off-by: Aditya Pakki <[email protected]>
> ---
>  net/sunrpc/auth_gss/auth_gss.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/auth_gss/auth_gss.c
> b/net/sunrpc/auth_gss/auth_gss.c
> index 5f42aa5fc612..eb52eebb3923 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -848,7 +848,8 @@ gss_pipe_destroy_msg(struct rpc_pipe_msg *msg)
>                         warn_gssd();
>                 gss_release_msg(gss_msg);
>         }
> -       gss_release_msg(gss_msg);
> +       if (gss_msg)
> +               gss_release_msg(gss_msg);
>  }
>  
>  static void gss_pipe_dentry_destroy(struct dentry *dir,


NACK. There's no double free there.

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]


2021-04-08 15:25:42

by Olga Kornievskaia

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Thu, Apr 8, 2021 at 11:01 AM Trond Myklebust <[email protected]> wrote:
>
> On Tue, 2021-04-06 at 19:16 -0500, Aditya Pakki wrote:
> > In gss_pipe_destroy_msg(), in case of error in msg, gss_release_msg
> > deletes gss_msg. The patch adds a check to avoid a potential double
> > free.
> >
> > Signed-off-by: Aditya Pakki <[email protected]>
> > ---
> > net/sunrpc/auth_gss/auth_gss.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/sunrpc/auth_gss/auth_gss.c
> > b/net/sunrpc/auth_gss/auth_gss.c
> > index 5f42aa5fc612..eb52eebb3923 100644
> > --- a/net/sunrpc/auth_gss/auth_gss.c
> > +++ b/net/sunrpc/auth_gss/auth_gss.c
> > @@ -848,7 +848,8 @@ gss_pipe_destroy_msg(struct rpc_pipe_msg *msg)
> > warn_gssd();
> > gss_release_msg(gss_msg);
> > }
> > - gss_release_msg(gss_msg);
> > + if (gss_msg)
> > + gss_release_msg(gss_msg);
> > }
> >
> > static void gss_pipe_dentry_destroy(struct dentry *dir,
>
>
> NACK. There's no double free there.

I disagree that there is no double free, the wording of the commit
describes the problem in the error case is that we call
gss_release_msg() and then we call it again but the 1st one released
the gss_msg. However, I think the fix should probably be instead:
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 5f42aa5fc612..e8aae617e981 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -846,7 +846,7 @@ gss_pipe_destroy_msg(struct rpc_pipe_msg *msg)
gss_unhash_msg(gss_msg);
if (msg->errno == -ETIMEDOUT)
warn_gssd();
- gss_release_msg(gss_msg);
+ return gss_release_msg(gss_msg);
}
gss_release_msg(gss_msg);
}

>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> [email protected]
>
>

2021-04-08 16:03:16

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Thu, 2021-04-08 at 11:24 -0400, Olga Kornievskaia wrote:
> On Thu, Apr 8, 2021 at 11:01 AM Trond Myklebust <
> [email protected]> wrote:
> >
> > On Tue, 2021-04-06 at 19:16 -0500, Aditya Pakki wrote:
> > > In gss_pipe_destroy_msg(), in case of error in msg,
> > > gss_release_msg
> > > deletes gss_msg. The patch adds a check to avoid a potential
> > > double
> > > free.
> > >
> > > Signed-off-by: Aditya Pakki <[email protected]>
> > > ---
> > >  net/sunrpc/auth_gss/auth_gss.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/sunrpc/auth_gss/auth_gss.c
> > > b/net/sunrpc/auth_gss/auth_gss.c
> > > index 5f42aa5fc612..eb52eebb3923 100644
> > > --- a/net/sunrpc/auth_gss/auth_gss.c
> > > +++ b/net/sunrpc/auth_gss/auth_gss.c
> > > @@ -848,7 +848,8 @@ gss_pipe_destroy_msg(struct rpc_pipe_msg
> > > *msg)
> > >                         warn_gssd();
> > >                 gss_release_msg(gss_msg);
> > >         }
> > > -       gss_release_msg(gss_msg);
> > > +       if (gss_msg)
> > > +               gss_release_msg(gss_msg);
> > >  }
> > >
> > >  static void gss_pipe_dentry_destroy(struct dentry *dir,
> >
> >
> > NACK. There's no double free there.
>
> I disagree that there is no double free, the wording of the commit
> describes the problem in the error case is that we call
> gss_release_msg() and then we call it again but the 1st one released
> the gss_msg. However, I think the fix should probably be instead:
> diff --git a/net/sunrpc/auth_gss/auth_gss.c
> b/net/sunrpc/auth_gss/auth_gss.c
> index 5f42aa5fc612..e8aae617e981 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -846,7 +846,7 @@ gss_pipe_destroy_msg(struct rpc_pipe_msg *msg)
>                 gss_unhash_msg(gss_msg);
>                 if (msg->errno == -ETIMEDOUT)
>                         warn_gssd();
> -               gss_release_msg(gss_msg);
> +               return gss_release_msg(gss_msg);
>         }
>         gss_release_msg(gss_msg);
>  }
>
Please look one line further up: there is a refcount_inc() that matches
that first gss_release_msg(). Removing that call results in a
reintroduction of the leak that Neil fixed in commit 1cded9d2974fe
("SUNRPC: fix refcounting problems with auth_gss messages.").

We might, however, instead opt to remove both the refcount_inc() and
matching gss_release_msg(). Those do seem to be redundant, given that
the caller also holds a refcount.

> >
> > --
> > Trond Myklebust
> > Linux NFS client maintainer, Hammerspace
> > [email protected]
> >
> >

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]


2021-04-20 07:16:09

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Tue, Apr 06, 2021 at 07:16:56PM -0500, Aditya Pakki wrote:
> In gss_pipe_destroy_msg(), in case of error in msg, gss_release_msg
> deletes gss_msg. The patch adds a check to avoid a potential double
> free.
>
> Signed-off-by: Aditya Pakki <[email protected]>
> ---
> net/sunrpc/auth_gss/auth_gss.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> index 5f42aa5fc612..eb52eebb3923 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -848,7 +848,8 @@ gss_pipe_destroy_msg(struct rpc_pipe_msg *msg)
> warn_gssd();
> gss_release_msg(gss_msg);
> }
> - gss_release_msg(gss_msg);
> + if (gss_msg)
> + gss_release_msg(gss_msg);a

If you look at the code, this is impossible to have happen.

Please stop submitting known-invalid patches. Your professor is playing
around with the review process in order to achieve a paper in some
strange and bizarre way.

This is not ok, it is wasting our time, and we will have to report this,
AGAIN, to your university...

greg k-h

2021-04-20 17:10:51

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Tue, Apr 20, 2021 at 09:15:23AM +0200, Greg KH wrote:
> If you look at the code, this is impossible to have happen.
>
> Please stop submitting known-invalid patches. Your professor is playing
> around with the review process in order to achieve a paper in some
> strange and bizarre way.
>
> This is not ok, it is wasting our time, and we will have to report this,
> AGAIN, to your university...

What's the story here?

--b.

2021-04-21 05:26:24

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Tue, Apr 20, 2021 at 01:10:08PM -0400, J. Bruce Fields wrote:
> On Tue, Apr 20, 2021 at 09:15:23AM +0200, Greg KH wrote:
> > If you look at the code, this is impossible to have happen.
> >
> > Please stop submitting known-invalid patches. Your professor is playing
> > around with the review process in order to achieve a paper in some
> > strange and bizarre way.
> >
> > This is not ok, it is wasting our time, and we will have to report this,
> > AGAIN, to your university...
>
> What's the story here?

Those commits are part of the following research:
https://github.com/QiushiWu/QiushiWu.github.io/blob/main/papers/OpenSourceInsecurity.pdf

They introduce kernel bugs on purpose. Yesterday, I took a look on 4
accepted patches from Aditya and 3 of them added various severity security
"holes".

Thanks

>
> --b.

2021-04-21 05:43:37

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 08:10:25AM +0300, Leon Romanovsky wrote:
> On Tue, Apr 20, 2021 at 01:10:08PM -0400, J. Bruce Fields wrote:
> > On Tue, Apr 20, 2021 at 09:15:23AM +0200, Greg KH wrote:
> > > If you look at the code, this is impossible to have happen.
> > >
> > > Please stop submitting known-invalid patches. Your professor is playing
> > > around with the review process in order to achieve a paper in some
> > > strange and bizarre way.
> > >
> > > This is not ok, it is wasting our time, and we will have to report this,
> > > AGAIN, to your university...
> >
> > What's the story here?
>
> Those commits are part of the following research:
> https://github.com/QiushiWu/QiushiWu.github.io/blob/main/papers/OpenSourceInsecurity.pdf
>
> They introduce kernel bugs on purpose. Yesterday, I took a look on 4
> accepted patches from Aditya and 3 of them added various severity security
> "holes".

All contributions by this group of people need to be reverted, if they
have not been done so already, as what they are doing is intentional
malicious behavior and is not acceptable and totally unethical. I'll
look at it after lunch unless someone else wants to do it...

greg k-h

2021-04-21 06:24:01

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 07:43:03AM +0200, Greg KH wrote:
> On Wed, Apr 21, 2021 at 08:10:25AM +0300, Leon Romanovsky wrote:
> > On Tue, Apr 20, 2021 at 01:10:08PM -0400, J. Bruce Fields wrote:
> > > On Tue, Apr 20, 2021 at 09:15:23AM +0200, Greg KH wrote:
> > > > If you look at the code, this is impossible to have happen.
> > > >
> > > > Please stop submitting known-invalid patches. Your professor is playing
> > > > around with the review process in order to achieve a paper in some
> > > > strange and bizarre way.
> > > >
> > > > This is not ok, it is wasting our time, and we will have to report this,
> > > > AGAIN, to your university...
> > >
> > > What's the story here?
> >
> > Those commits are part of the following research:
> > https://github.com/QiushiWu/QiushiWu.github.io/blob/main/papers/OpenSourceInsecurity.pdf
> >
> > They introduce kernel bugs on purpose. Yesterday, I took a look on 4
> > accepted patches from Aditya and 3 of them added various severity security
> > "holes".
>
> All contributions by this group of people need to be reverted, if they
> have not been done so already, as what they are doing is intentional
> malicious behavior and is not acceptable and totally unethical. I'll
> look at it after lunch unless someone else wants to do it...

It looks like the best possible scenario.
I asked to revert the bad patch, but didn't get any response yet.
https://lore.kernel.org/lkml/YH6aMsbqruMZiWFe@unreal/

>
> greg k-h

2021-04-21 08:16:52

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Wed, Apr 21, 2021 at 02:56:27AM -0500, Aditya Pakki wrote:
> Greg,
>
> I respectfully ask you to cease and desist from making wild accusations
> that are bordering on slander.
>
> These patches were sent as part of a new static analyzer that I wrote and
> it's sensitivity is obviously not great. I sent patches on the hopes to get
> feedback. We are not experts in the linux kernel and repeatedly making
> these statements is disgusting to hear.
>
> Obviously, it is a wrong step but your preconceived biases are so strong
> that you make allegations without merit nor give us any benefit of doubt.
>
> I will not be sending any more patches due to the attitude that is not only
> unwelcome but also intimidating to newbies and non experts.

You, and your group, have publicly admitted to sending known-buggy
patches to see how the kernel community would react to them, and
published a paper based on that work.

Now you submit a new series of obviously-incorrect patches again, so
what am I supposed to think of such a thing?

They obviously were _NOT_ created by a static analysis tool that is of
any intelligence, as they all are the result of totally different
patterns, and all of which are obviously not even fixing anything at
all. So what am I supposed to think here, other than that you and your
group are continuing to experiment on the kernel community developers by
sending such nonsense patches?

When submitting patches created by a tool, everyone who does so submits
them with wording like "found by tool XXX, we are not sure if this is
correct or not, please advise." which is NOT what you did here at all.
You were not asking for help, you were claiming that these were
legitimate fixes, which you KNEW to be incorrect.

A few minutes with anyone with the semblance of knowledge of C can see
that your submissions do NOT do anything at all, so to think that a tool
created them, and then that you thought they were a valid "fix" is
totally negligent on your part, not ours. You are the one at fault, it
is not our job to be the test subjects of a tool you create.

Our community welcomes developers who wish to help and enhance Linux.
That is NOT what you are attempting to do here, so please do not try to
frame it that way.

Our community does not appreciate being experimented on, and being
"tested" by submitting known patches that are either do nothing on
purpose, or introduce bugs on purpose. If you wish to do work like
this, I suggest you find a different community to run your experiments
on, you are not welcome here.

Because of this, I will now have to ban all future contributions from
your University and rip out your previous contributions, as they were
obviously submitted in bad-faith with the intent to cause problems.

*plonk*

greg k-h

2021-04-21 10:28:54

by Sudip Mukherjee

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

Hi Greg,

On Wed, Apr 21, 2021 at 6:44 AM Greg KH <[email protected]> wrote:
>
> On Wed, Apr 21, 2021 at 08:10:25AM +0300, Leon Romanovsky wrote:
> > On Tue, Apr 20, 2021 at 01:10:08PM -0400, J. Bruce Fields wrote:
> > > On Tue, Apr 20, 2021 at 09:15:23AM +0200, Greg KH wrote:
> > > > If you look at the code, this is impossible to have happen.
> > > >

<snip>

> > They introduce kernel bugs on purpose. Yesterday, I took a look on 4
> > accepted patches from Aditya and 3 of them added various severity security
> > "holes".
>
> All contributions by this group of people need to be reverted, if they
> have not been done so already, as what they are doing is intentional
> malicious behavior and is not acceptable and totally unethical. I'll
> look at it after lunch unless someone else wants to do it...

A lot of these have already reached the stable trees. I can send you
revert patches for stable by the end of today (if your scripts have
not already done it).


--
Regards
Sudip

2021-04-21 10:32:43

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 11:07:11AM +0100, Sudip Mukherjee wrote:
> Hi Greg,
>
> On Wed, Apr 21, 2021 at 6:44 AM Greg KH <[email protected]> wrote:
> >
> > On Wed, Apr 21, 2021 at 08:10:25AM +0300, Leon Romanovsky wrote:
> > > On Tue, Apr 20, 2021 at 01:10:08PM -0400, J. Bruce Fields wrote:
> > > > On Tue, Apr 20, 2021 at 09:15:23AM +0200, Greg KH wrote:
> > > > > If you look at the code, this is impossible to have happen.
> > > > >
>
> <snip>
>
> > > They introduce kernel bugs on purpose. Yesterday, I took a look on 4
> > > accepted patches from Aditya and 3 of them added various severity security
> > > "holes".
> >
> > All contributions by this group of people need to be reverted, if they
> > have not been done so already, as what they are doing is intentional
> > malicious behavior and is not acceptable and totally unethical. I'll
> > look at it after lunch unless someone else wants to do it...
>
> A lot of these have already reached the stable trees. I can send you
> revert patches for stable by the end of today (if your scripts have
> not already done it).

Yes, if you have a list of these that are already in the stable trees,
that would be great to have revert patches, it would save me the extra
effort these mess is causing us to have to do...

thanks,

greg k-h

2021-04-21 13:02:20

by Shelat, Abhi

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

>>
>>>> They introduce kernel bugs on purpose. Yesterday, I took a look on 4
>>>> accepted patches from Aditya and 3 of them added various severity security
>>>> "holes".
>>>
>>> All contributions by this group of people need to be reverted, if they
>>> have not been done so already, as what they are doing is intentional
>>> malicious behavior and is not acceptable and totally unethical. I'll
>>> look at it after lunch unless someone else wants to do it…
>>

<snip>

Academic research should NOT waste the time of a community.

If you believe this behavior deserves an escalation, you can contact the Institutional Review Board ([email protected]) at UMN to investigate whether this behavior was harmful; in particular, whether the research activity had an appropriate IRB review, and what safeguards prevent repeats in other communities.

All researchers at UMN must comply with their Human Research Protection Program Plan [1], and the UMN worksheet [2] to determine if a research activity needs IRB approval includes this question:

====
Will the investigator use, study, or analyze information or biospecimens obtained through either of the following mechanisms,? Specify
which mechanism(s) apply, if yes:

Communication or interpersonal contact with the individuals. ("interaction”).
===

which I believe is true based on this thread.


[1] Human Research Protection Program Plan
https://drive.google.com/file/d/0B7644h9N2vLcV3FyMzJKYnJGeDA/view
[2] Human Research Determination
https://drive.google.com/file/d/0Bw4LRE9kGb69Mm5TbldxSVkwTms/view

2021-04-21 13:03:19

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 11:58:08AM +0000, Shelat, Abhi wrote:
> >>
> >>>> They introduce kernel bugs on purpose. Yesterday, I took a look on 4
> >>>> accepted patches from Aditya and 3 of them added various severity security
> >>>> "holes".
> >>>
> >>> All contributions by this group of people need to be reverted, if they
> >>> have not been done so already, as what they are doing is intentional
> >>> malicious behavior and is not acceptable and totally unethical. I'll
> >>> look at it after lunch unless someone else wants to do it…
> >>
>
> <snip>
>
> Academic research should NOT waste the time of a community.

Thank you for saying this, I appreciate it.

greg k-h

2021-04-21 13:03:34

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 11:58:08AM +0000, Shelat, Abhi wrote:
> >>
> >>>> They introduce kernel bugs on purpose. Yesterday, I took a look on 4
> >>>> accepted patches from Aditya and 3 of them added various severity security
> >>>> "holes".
> >>>
> >>> All contributions by this group of people need to be reverted, if they
> >>> have not been done so already, as what they are doing is intentional
> >>> malicious behavior and is not acceptable and totally unethical. I'll
> >>> look at it after lunch unless someone else wants to do it…
> >>
>
> <snip>
>
> Academic research should NOT waste the time of a community.
>
> If you believe this behavior deserves an escalation, you can contact the Institutional Review Board ([email protected]) at UMN to investigate whether this behavior was harmful; in particular, whether the research activity had an appropriate IRB review, and what safeguards prevent repeats in other communities.

The huge advantage of being "community" is that we don't need to do all
the above and waste our time to fill some bureaucratic forms with unclear
timelines and results.

Our solution to ignore all @umn.edu contributions is much more reliable
to us who are suffering from these researchers.

Thanks

2021-04-21 13:04:57

by Anna Schumaker

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 2:07 AM Leon Romanovsky <[email protected]> wrote:
>
> On Tue, Apr 20, 2021 at 01:10:08PM -0400, J. Bruce Fields wrote:
> > On Tue, Apr 20, 2021 at 09:15:23AM +0200, Greg KH wrote:
> > > If you look at the code, this is impossible to have happen.
> > >
> > > Please stop submitting known-invalid patches. Your professor is playing
> > > around with the review process in order to achieve a paper in some
> > > strange and bizarre way.
> > >
> > > This is not ok, it is wasting our time, and we will have to report this,
> > > AGAIN, to your university...
> >
> > What's the story here?
>
> Those commits are part of the following research:
> https://github.com/QiushiWu/QiushiWu.github.io/blob/main/papers/OpenSourceInsecurity.pdf

This thread is the first I'm hearing about this. I wonder if there is
a good way of alerting the entire kernel community (including those
only subscribed to subsystem mailing lists) about what's going on? It
seems like useful information to have to push back against these
patches.

Anna

>
> They introduce kernel bugs on purpose. Yesterday, I took a look on 4
> accepted patches from Aditya and 3 of them added various severity security
> "holes".
>
> Thanks
>
> >
> > --b.

2021-04-21 13:17:09

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, 2021-04-21 at 15:19 +0300, Leon Romanovsky wrote:
> On Wed, Apr 21, 2021 at 11:58:08AM +0000, Shelat, Abhi wrote:
> > > >
> > > > > > They introduce kernel bugs on purpose. Yesterday, I took a
> > > > > > look on 4
> > > > > > accepted patches from Aditya and 3 of them added various
> > > > > > severity security
> > > > > > "holes".
> > > > >
> > > > > All contributions by this group of people need to be
> > > > > reverted, if they
> > > > > have not been done so already, as what they are doing is
> > > > > intentional
> > > > > malicious behavior and is not acceptable and totally
> > > > > unethical.  I'll
> > > > > look at it after lunch unless someone else wants to do it…
> > > >
> >
> > <snip>
> >
> > Academic research should NOT waste the time of a community.
> >
> > If you believe this behavior deserves an escalation, you can
> > contact the Institutional Review Board ([email protected]) at UMN to
> > investigate whether this behavior was harmful; in particular,
> > whether the research activity had an appropriate IRB review, and
> > what safeguards prevent repeats in other communities.
>
> The huge advantage of being "community" is that we don't need to do
> all
> the above and waste our time to fill some bureaucratic forms with
> unclear
> timelines and results.
>
> Our solution to ignore all @umn.edu contributions is much more
> reliable
> to us who are suffering from these researchers.
>

<shrug>That's an easy thing to sidestep by just shifting to using a
private email address.</shrug>

There really is no alternative for maintainers other than to always be
sceptical of patches submitted by people who are not known and trusted
members of the community, and to scrutinise those patches with more
care.

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]


2021-04-21 13:22:56

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 01:11:03PM +0000, Trond Myklebust wrote:
> On Wed, 2021-04-21 at 15:19 +0300, Leon Romanovsky wrote:
> > On Wed, Apr 21, 2021 at 11:58:08AM +0000, Shelat, Abhi wrote:
> > > > >
> > > > > > > They introduce kernel bugs on purpose. Yesterday, I took a
> > > > > > > look on 4
> > > > > > > accepted patches from Aditya and 3 of them added various
> > > > > > > severity security
> > > > > > > "holes".
> > > > > >
> > > > > > All contributions by this group of people need to be
> > > > > > reverted, if they
> > > > > > have not been done so already, as what they are doing is
> > > > > > intentional
> > > > > > malicious behavior and is not acceptable and totally
> > > > > > unethical.  I'll
> > > > > > look at it after lunch unless someone else wants to do it…
> > > > >
> > >
> > > <snip>
> > >
> > > Academic research should NOT waste the time of a community.
> > >
> > > If you believe this behavior deserves an escalation, you can
> > > contact the Institutional Review Board ([email protected]) at UMN to
> > > investigate whether this behavior was harmful; in particular,
> > > whether the research activity had an appropriate IRB review, and
> > > what safeguards prevent repeats in other communities.
> >
> > The huge advantage of being "community" is that we don't need to do
> > all
> > the above and waste our time to fill some bureaucratic forms with
> > unclear
> > timelines and results.
> >
> > Our solution to ignore all @umn.edu contributions is much more
> > reliable
> > to us who are suffering from these researchers.
> >
>
> <shrug>That's an easy thing to sidestep by just shifting to using a
> private email address.</shrug>
>
> There really is no alternative for maintainers other than to always be
> sceptical of patches submitted by people who are not known and trusted
> members of the community, and to scrutinise those patches with more
> care.

Right, my guess is that many maintainers failed in the trap when they
saw respectful address @umn.edu together with commit message saying
about "new static analyzer tool".

The mental bias here is to say that "oh, another academic group tries
to reinvent the wheel, looks ok".

Thanks

>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> [email protected]
>
>

2021-04-21 13:23:03

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 01:11:03PM +0000, Trond Myklebust wrote:
> On Wed, 2021-04-21 at 15:19 +0300, Leon Romanovsky wrote:
> > On Wed, Apr 21, 2021 at 11:58:08AM +0000, Shelat, Abhi wrote:
> > > > >
> > > > > > > They introduce kernel bugs on purpose. Yesterday, I took a
> > > > > > > look on 4
> > > > > > > accepted patches from Aditya and 3 of them added various
> > > > > > > severity security
> > > > > > > "holes".
> > > > > >
> > > > > > All contributions by this group of people need to be
> > > > > > reverted, if they
> > > > > > have not been done so already, as what they are doing is
> > > > > > intentional
> > > > > > malicious behavior and is not acceptable and totally
> > > > > > unethical.  I'll
> > > > > > look at it after lunch unless someone else wants to do it…
> > > > >
> > >
> > > <snip>
> > >
> > > Academic research should NOT waste the time of a community.
> > >
> > > If you believe this behavior deserves an escalation, you can
> > > contact the Institutional Review Board ([email protected]) at UMN to
> > > investigate whether this behavior was harmful; in particular,
> > > whether the research activity had an appropriate IRB review, and
> > > what safeguards prevent repeats in other communities.
> >
> > The huge advantage of being "community" is that we don't need to do
> > all
> > the above and waste our time to fill some bureaucratic forms with
> > unclear
> > timelines and results.
> >
> > Our solution to ignore all @umn.edu contributions is much more
> > reliable
> > to us who are suffering from these researchers.
> >
>
> <shrug>That's an easy thing to sidestep by just shifting to using a
> private email address.</shrug>

If they just want to be jerks, yes. But they can't then use that type
of "hiding" to get away with claiming it was done for a University
research project as that's even more unethical than what they are doing
now.

> There really is no alternative for maintainers other than to always be
> sceptical of patches submitted by people who are not known and trusted
> members of the community, and to scrutinise those patches with more
> care.

Agreed, and when we notice things like this that were determined to be
bad, we have the ability to easily go back and rip the changes out and
we can slowly add them back if they are actually something we want to
do.

Which is what I just did:
https://lore.kernel.org/lkml/[email protected]/

thanks,

greg k-h

2021-04-21 17:22:20

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 03:21:15PM +0200, [email protected] wrote:
> On Wed, Apr 21, 2021 at 01:11:03PM +0000, Trond Myklebust wrote:
> > On Wed, 2021-04-21 at 15:19 +0300, Leon Romanovsky wrote:
> > > On Wed, Apr 21, 2021 at 11:58:08AM +0000, Shelat, Abhi wrote:
> > > > > >
> > > > > > > > They introduce kernel bugs on purpose. Yesterday, I took a
> > > > > > > > look on 4
> > > > > > > > accepted patches from Aditya and 3 of them added various
> > > > > > > > severity security
> > > > > > > > "holes".
> > > > > > >
> > > > > > > All contributions by this group of people need to be
> > > > > > > reverted, if they
> > > > > > > have not been done so already, as what they are doing is
> > > > > > > intentional
> > > > > > > malicious behavior and is not acceptable and totally
> > > > > > > unethical.  I'll
> > > > > > > look at it after lunch unless someone else wants to do it…
> > > > > >
> > > >
> > > > <snip>
> > > >
> > > > Academic research should NOT waste the time of a community.
> > > >
> > > > If you believe this behavior deserves an escalation, you can
> > > > contact the Institutional Review Board ([email protected]) at UMN to
> > > > investigate whether this behavior was harmful; in particular,
> > > > whether the research activity had an appropriate IRB review, and
> > > > what safeguards prevent repeats in other communities.
> > >
> > > The huge advantage of being "community" is that we don't need to do
> > > all
> > > the above and waste our time to fill some bureaucratic forms with
> > > unclear
> > > timelines and results.
> > >
> > > Our solution to ignore all @umn.edu contributions is much more
> > > reliable
> > > to us who are suffering from these researchers.
> > >
> >
> > <shrug>That's an easy thing to sidestep by just shifting to using a
> > private email address.</shrug>
>
> If they just want to be jerks, yes. But they can't then use that type
> of "hiding" to get away with claiming it was done for a University
> research project as that's even more unethical than what they are doing
> now.
>
> > There really is no alternative for maintainers other than to always be
> > sceptical of patches submitted by people who are not known and trusted
> > members of the community, and to scrutinise those patches with more
> > care.
>
> Agreed, and when we notice things like this that were determined to be
> bad, we have the ability to easily go back and rip the changes out and
> we can slowly add them back if they are actually something we want to
> do.
>
> Which is what I just did:
> https://lore.kernel.org/lkml/[email protected]/

Greg,

Did you push your series to the public git? I would like to add you a
couple of reverts.

And do you have a list of not reverted commits? It will save us from
doing same comparison of reverted/not reverted over and over.

Thanks

>
> thanks,
>
> greg k-h

2021-04-21 17:24:59

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 11:58:08AM +0000, Shelat, Abhi wrote:
> Academic research should NOT waste the time of a community.
>
> If you believe this behavior deserves an escalation, you can contact
> the Institutional Review Board ([email protected]) at UMN to investigate
> whether this behavior was harmful; in particular, whether the research
> activity had an appropriate IRB review, and what safeguards prevent
> repeats in other communities.

For what it's worth, they do address security, IRB, and maintainer-time
questions in "Ethical Considerations", starting on p. 8:

https://github.com/QiushiWu/QiushiWu.github.io/blob/main/papers/OpenSourceInsecurity.pdf

(Summary: in that experiment, they claim actual fixes were sent before
the original (incorrect) patches had a chance to be committed; that
their IRB reviewed the plan and determined it was not human research;
and that patches were all small and (after correction) fixed real (if
minor) bugs.)

This effort doesn't appear to be following similar protocols, if Leon
Romanvosky and Aditya Pakki are correct that security holes have already
reached stable.

Also, I still don't understand the explanation of the original SUNRPC
patch. I don't know much about static analyzers, but it really doesn't
look like the kind of mistake I'd expect one to make.

--b.

2021-04-21 17:27:42

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, 21 Apr 2021 16:20:46 +0300
Leon Romanovsky <[email protected]> wrote:

> > There really is no alternative for maintainers other than to always be
> > sceptical of patches submitted by people who are not known and trusted
> > members of the community, and to scrutinise those patches with more
> > care.
>

There's only a couple of contributors to my code that I will take without
looking deeply at what it does. And those are well respected developers
that many other people know.

> Right, my guess is that many maintainers failed in the trap when they
> saw respectful address @umn.edu together with commit message saying
> about "new static analyzer tool".
>
> The mental bias here is to say that "oh, another academic group tries
> to reinvent the wheel, looks ok".

I'm skeptical of all static analyzers, as I've seen too many good ones
still produce crappy fixes. I look even more carefully if I see that it was
a tool that discovered the bug and not a human.

The one patch from Greg's reverts that affects my code was actually a
legitimate fix, and looking back at the thread of the submission, I even
asked if it was found via inspection or a tool.

https://lore.kernel.org/lkml/[email protected]/

-- Steve

2021-04-21 17:30:48

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 09:37:27AM -0400, J. Bruce Fields wrote:
> On Wed, Apr 21, 2021 at 11:58:08AM +0000, Shelat, Abhi wrote:
> > Academic research should NOT waste the time of a community.
> >
> > If you believe this behavior deserves an escalation, you can contact
> > the Institutional Review Board ([email protected]) at UMN to investigate
> > whether this behavior was harmful; in particular, whether the research
> > activity had an appropriate IRB review, and what safeguards prevent
> > repeats in other communities.
>
> For what it's worth, they do address security, IRB, and maintainer-time
> questions in "Ethical Considerations", starting on p. 8:
>
> https://github.com/QiushiWu/QiushiWu.github.io/blob/main/papers/OpenSourceInsecurity.pdf
>
> (Summary: in that experiment, they claim actual fixes were sent before
> the original (incorrect) patches had a chance to be committed; that
> their IRB reviewed the plan and determined it was not human research;
> and that patches were all small and (after correction) fixed real (if
> minor) bugs.)
>
> This effort doesn't appear to be following similar protocols, if Leon
> Romanvosky and Aditya Pakki are correct that security holes have already
> reached stable.

Aditya Pakki is the one who is sending those patches.

If you want to see another accepted patch that is already part of
stable@, you are invited to take a look on this patch that has "built-in bug":
8e949363f017 ("net: mlx5: Add a missing check on idr_find, free buf")

Thanks

2021-04-21 17:33:12

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 04:34:00PM +0300, Leon Romanovsky wrote:
> On Wed, Apr 21, 2021 at 03:21:15PM +0200, [email protected] wrote:
> > On Wed, Apr 21, 2021 at 01:11:03PM +0000, Trond Myklebust wrote:
> > > On Wed, 2021-04-21 at 15:19 +0300, Leon Romanovsky wrote:
> > > > On Wed, Apr 21, 2021 at 11:58:08AM +0000, Shelat, Abhi wrote:
> > > > > > >
> > > > > > > > > They introduce kernel bugs on purpose. Yesterday, I took a
> > > > > > > > > look on 4
> > > > > > > > > accepted patches from Aditya and 3 of them added various
> > > > > > > > > severity security
> > > > > > > > > "holes".
> > > > > > > >
> > > > > > > > All contributions by this group of people need to be
> > > > > > > > reverted, if they
> > > > > > > > have not been done so already, as what they are doing is
> > > > > > > > intentional
> > > > > > > > malicious behavior and is not acceptable and totally
> > > > > > > > unethical.  I'll
> > > > > > > > look at it after lunch unless someone else wants to do it…
> > > > > > >
> > > > >
> > > > > <snip>
> > > > >
> > > > > Academic research should NOT waste the time of a community.
> > > > >
> > > > > If you believe this behavior deserves an escalation, you can
> > > > > contact the Institutional Review Board ([email protected]) at UMN to
> > > > > investigate whether this behavior was harmful; in particular,
> > > > > whether the research activity had an appropriate IRB review, and
> > > > > what safeguards prevent repeats in other communities.
> > > >
> > > > The huge advantage of being "community" is that we don't need to do
> > > > all
> > > > the above and waste our time to fill some bureaucratic forms with
> > > > unclear
> > > > timelines and results.
> > > >
> > > > Our solution to ignore all @umn.edu contributions is much more
> > > > reliable
> > > > to us who are suffering from these researchers.
> > > >
> > >
> > > <shrug>That's an easy thing to sidestep by just shifting to using a
> > > private email address.</shrug>
> >
> > If they just want to be jerks, yes. But they can't then use that type
> > of "hiding" to get away with claiming it was done for a University
> > research project as that's even more unethical than what they are doing
> > now.
> >
> > > There really is no alternative for maintainers other than to always be
> > > sceptical of patches submitted by people who are not known and trusted
> > > members of the community, and to scrutinise those patches with more
> > > care.
> >
> > Agreed, and when we notice things like this that were determined to be
> > bad, we have the ability to easily go back and rip the changes out and
> > we can slowly add them back if they are actually something we want to
> > do.
> >
> > Which is what I just did:
> > https://lore.kernel.org/lkml/[email protected]/
>
> Greg,
>
> Did you push your series to the public git? I would like to add you a
> couple of reverts.

Yes, it can be found here:
[email protected]:/pub/scm/linux/kernel/git/gregkh/driver-core.git umn.edu-reverts

You can send reverts in email if you want, whatever works best.

> And do you have a list of not reverted commits? It will save us from
> doing same comparison of reverted/not reverted over and over.

Below is the list that didn't do a simple "revert" that I need to look
at. I was going to have my interns look into this, there's no need to
bother busy maintainers with it unless you really want to, as I can't
tell anyone what to work on :)

thanks,

greg k-h

-------------------------
# commits that need to be looked at as a clean revert did not work
990a1162986e
58d0c864e1a7
a068aab42258
8816cd726a4f
c705f9fc6a17
8b6fc114beeb
169f9acae086
8da96730331d
f4f5748bfec9
e08f0761234d
cb5173594d50
06d5d6b7f994
d9350f21e5fe
6f0ce4dfc5a3
f0d14edd2ba4
46953f97224d
3c77ff8f8bae
0aab8e4df470
8e949363f017
f8ee34c3e77a
fd21b79e541e
766460852cfa
41f00e6e9e55
78540a259b05
208c6e8cff1b
7ecced0934e5
48f40b96de2c
9aabb68568b4
2cc12751cf46
534c89c22e26
6a8ca24590a2
d70d70aec963
d7737d425745
3a10e3dd52e8
d6cb77228e3a
517ccc2aa50d
07660ca679da
0fff9bd47e13
6ade657d6125
2795e8c25161
4ec850e5dfec
035a14e71f27
10010493c126
4280b73092fe
5910fa0d0d98
40619f7dd3ef
0a54ea9f481f
44fabd8cdaaa
02cc53e223d4
c99776cc4018
7fc93f3285b1
6ae16dfb61bc
9c6260de505b
eb8950861c1b
46273cf7e009
89dfd0083751
c9c63915519b
cd07e3701fa6
15b3048aeed8
7172122be6a4
47db7873136a
58f5bbe331c5
6b995f4eec34
8af03d1ae2e1
f16b613ca8b3
6009d1fe6ba3
8e03477cb709
dc487321b1e6

2021-04-21 17:38:05

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 04:49:31PM +0300, Leon Romanovsky wrote:
> On Wed, Apr 21, 2021 at 09:37:27AM -0400, J. Bruce Fields wrote:
> > On Wed, Apr 21, 2021 at 11:58:08AM +0000, Shelat, Abhi wrote:
> > > Academic research should NOT waste the time of a community.
> > >
> > > If you believe this behavior deserves an escalation, you can contact
> > > the Institutional Review Board ([email protected]) at UMN to investigate
> > > whether this behavior was harmful; in particular, whether the research
> > > activity had an appropriate IRB review, and what safeguards prevent
> > > repeats in other communities.
> >
> > For what it's worth, they do address security, IRB, and maintainer-time
> > questions in "Ethical Considerations", starting on p. 8:
> >
> > https://github.com/QiushiWu/QiushiWu.github.io/blob/main/papers/OpenSourceInsecurity.pdf
> >
> > (Summary: in that experiment, they claim actual fixes were sent before
> > the original (incorrect) patches had a chance to be committed; that
> > their IRB reviewed the plan and determined it was not human research;
> > and that patches were all small and (after correction) fixed real (if
> > minor) bugs.)
> >
> > This effort doesn't appear to be following similar protocols, if Leon
> > Romanvosky and Aditya Pakki are correct that security holes have already
> > reached stable.
>
> Aditya Pakki is the one who is sending those patches.

Argh, sorry, I I meant Sudip Mukherjee, who reported their reaching
stable. Apologies.

> If you want to see another accepted patch that is already part of
> stable@, you are invited to take a look on this patch that has "built-in bug":
> 8e949363f017 ("net: mlx5: Add a missing check on idr_find, free buf")

Interesting, thanks.

--b.

2021-04-21 17:48:47

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 03:50:58PM +0200, [email protected] wrote:
> On Wed, Apr 21, 2021 at 04:34:00PM +0300, Leon Romanovsky wrote:
> > On Wed, Apr 21, 2021 at 03:21:15PM +0200, [email protected] wrote:
> > > On Wed, Apr 21, 2021 at 01:11:03PM +0000, Trond Myklebust wrote:
> > > > On Wed, 2021-04-21 at 15:19 +0300, Leon Romanovsky wrote:
> > > > > On Wed, Apr 21, 2021 at 11:58:08AM +0000, Shelat, Abhi wrote:
> > > > > > > >
> > > > > > > > > > They introduce kernel bugs on purpose. Yesterday, I took a
> > > > > > > > > > look on 4
> > > > > > > > > > accepted patches from Aditya and 3 of them added various
> > > > > > > > > > severity security
> > > > > > > > > > "holes".
> > > > > > > > >
> > > > > > > > > All contributions by this group of people need to be
> > > > > > > > > reverted, if they
> > > > > > > > > have not been done so already, as what they are doing is
> > > > > > > > > intentional
> > > > > > > > > malicious behavior and is not acceptable and totally
> > > > > > > > > unethical.  I'll
> > > > > > > > > look at it after lunch unless someone else wants to do it…
> > > > > > > >
> > > > > >
> > > > > > <snip>
> > > > > >
> > > > > > Academic research should NOT waste the time of a community.
> > > > > >
> > > > > > If you believe this behavior deserves an escalation, you can
> > > > > > contact the Institutional Review Board ([email protected]) at UMN to
> > > > > > investigate whether this behavior was harmful; in particular,
> > > > > > whether the research activity had an appropriate IRB review, and
> > > > > > what safeguards prevent repeats in other communities.
> > > > >
> > > > > The huge advantage of being "community" is that we don't need to do
> > > > > all
> > > > > the above and waste our time to fill some bureaucratic forms with
> > > > > unclear
> > > > > timelines and results.
> > > > >
> > > > > Our solution to ignore all @umn.edu contributions is much more
> > > > > reliable
> > > > > to us who are suffering from these researchers.
> > > > >
> > > >
> > > > <shrug>That's an easy thing to sidestep by just shifting to using a
> > > > private email address.</shrug>
> > >
> > > If they just want to be jerks, yes. But they can't then use that type
> > > of "hiding" to get away with claiming it was done for a University
> > > research project as that's even more unethical than what they are doing
> > > now.
> > >
> > > > There really is no alternative for maintainers other than to always be
> > > > sceptical of patches submitted by people who are not known and trusted
> > > > members of the community, and to scrutinise those patches with more
> > > > care.
> > >
> > > Agreed, and when we notice things like this that were determined to be
> > > bad, we have the ability to easily go back and rip the changes out and
> > > we can slowly add them back if they are actually something we want to
> > > do.
> > >
> > > Which is what I just did:
> > > https://lore.kernel.org/lkml/[email protected]/
> >
> > Greg,
> >
> > Did you push your series to the public git? I would like to add you a
> > couple of reverts.
>
> Yes, it can be found here:
> [email protected]:/pub/scm/linux/kernel/git/gregkh/driver-core.git umn.edu-reverts
>
> You can send reverts in email if you want, whatever works best.
>
> > And do you have a list of not reverted commits? It will save us from
> > doing same comparison of reverted/not reverted over and over.
>
> Below is the list that didn't do a simple "revert" that I need to look
> at. I was going to have my interns look into this, there's no need to
> bother busy maintainers with it unless you really want to, as I can't
> tell anyone what to work on :)

Ohh, you have interns, so even better.

Thanks

>
> thanks,
>
> greg k-h
>
> -------------------------
> # commits that need to be looked at as a clean revert did not work
> 990a1162986e
> 58d0c864e1a7
> a068aab42258
> 8816cd726a4f
> c705f9fc6a17
> 8b6fc114beeb
> 169f9acae086
> 8da96730331d
> f4f5748bfec9
> e08f0761234d
> cb5173594d50
> 06d5d6b7f994
> d9350f21e5fe
> 6f0ce4dfc5a3
> f0d14edd2ba4
> 46953f97224d
> 3c77ff8f8bae
> 0aab8e4df470
> 8e949363f017
> f8ee34c3e77a
> fd21b79e541e
> 766460852cfa
> 41f00e6e9e55
> 78540a259b05
> 208c6e8cff1b
> 7ecced0934e5
> 48f40b96de2c
> 9aabb68568b4
> 2cc12751cf46
> 534c89c22e26
> 6a8ca24590a2
> d70d70aec963
> d7737d425745
> 3a10e3dd52e8
> d6cb77228e3a
> 517ccc2aa50d
> 07660ca679da
> 0fff9bd47e13
> 6ade657d6125
> 2795e8c25161
> 4ec850e5dfec
> 035a14e71f27
> 10010493c126
> 4280b73092fe
> 5910fa0d0d98
> 40619f7dd3ef
> 0a54ea9f481f
> 44fabd8cdaaa
> 02cc53e223d4
> c99776cc4018
> 7fc93f3285b1
> 6ae16dfb61bc
> 9c6260de505b
> eb8950861c1b
> 46273cf7e009
> 89dfd0083751
> c9c63915519b
> cd07e3701fa6
> 15b3048aeed8
> 7172122be6a4
> 47db7873136a
> 58f5bbe331c5
> 6b995f4eec34
> 8af03d1ae2e1
> f16b613ca8b3
> 6009d1fe6ba3
> 8e03477cb709
> dc487321b1e6

2021-04-21 17:50:11

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 08:51:02AM -0400, Anna Schumaker wrote:
> On Wed, Apr 21, 2021 at 2:07 AM Leon Romanovsky <[email protected]> wrote:
> >
> > On Tue, Apr 20, 2021 at 01:10:08PM -0400, J. Bruce Fields wrote:
> > > On Tue, Apr 20, 2021 at 09:15:23AM +0200, Greg KH wrote:
> > > > If you look at the code, this is impossible to have happen.
> > > >
> > > > Please stop submitting known-invalid patches. Your professor is playing
> > > > around with the review process in order to achieve a paper in some
> > > > strange and bizarre way.
> > > >
> > > > This is not ok, it is wasting our time, and we will have to report this,
> > > > AGAIN, to your university...
> > >
> > > What's the story here?
> >
> > Those commits are part of the following research:
> > https://github.com/QiushiWu/QiushiWu.github.io/blob/main/papers/OpenSourceInsecurity.pdf
>
> This thread is the first I'm hearing about this. I wonder if there is
> a good way of alerting the entire kernel community (including those
> only subscribed to subsystem mailing lists) about what's going on? It
> seems like useful information to have to push back against these
> patches.

IMHO, kernel users ML is good enough for that.

2021-04-21 23:38:42

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 05:15:26PM +0300, Leon Romanovsky wrote:
> > This thread is the first I'm hearing about this. I wonder if there is
> > a good way of alerting the entire kernel community (including those
> > only subscribed to subsystem mailing lists) about what's going on? It
> > seems like useful information to have to push back against these
> > patches.
>
> IMHO, kernel users ML is good enough for that.

The problem is that LKML is too high traffic for a lot of people to
want to follow.

There are some people who have used the kernel summit discuss list
(previously [email protected], now
[email protected]) as a place where most maintainers tend to be
subscribed, although that's not really a guarantee, either. (Speaking
of which, how to handle groups who submit patches in bad faith a good
Maintainer Summit topic for someone to propose...)

To give the devil his due, Prof. Kangjie Lu has reported legitimate
security issues in the past (CVE-2016-4482, an information leak from
the kernel stack in the core USB layer, and CVE-2016-4485, an
information leak in the 802.2 networking code), and if one looks at
his CV, he has a quite a few papers in the security area to his name.

The problem is that Prof. Lu and his team seem to be unrepentant, and
has some very... skewed... ideas over what is considered ethical, and
acceptable behavior vis-a-vis the Kernel development community. The
fact that the UMN IRB team believes that what Prof. Lu is doing isn't
considered in scope for human experimentation means that there isn't
any kind of institutional controls at UMN for this sort of behavior
--- which is why a University-wide Ban may be the only right answer,
unfortunately.

- Ted

2021-04-22 01:08:56

by Mike Rapoport

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 11:48:46AM -0400, Theodore Ts'o wrote:
> On Wed, Apr 21, 2021 at 05:15:26PM +0300, Leon Romanovsky wrote:
> > > This thread is the first I'm hearing about this. I wonder if there is
> > > a good way of alerting the entire kernel community (including those
> > > only subscribed to subsystem mailing lists) about what's going on? It
> > > seems like useful information to have to push back against these
> > > patches.

Heh, I've got this information from google news feed on my phone :)

> > IMHO, kernel users ML is good enough for that.
>
> The problem is that LKML is too high traffic for a lot of people to
> want to follow.

I think Leon meant kernel.org users ML ([email protected]). Along with
ksummut-discuss it'll reach most maintainers, IMHO.

> There are some people who have used the kernel summit discuss list
> (previously [email protected], now
> [email protected]) as a place where most maintainers tend to be
> subscribed, although that's not really a guarantee, either. (Speaking
> of which, how to handle groups who submit patches in bad faith a good
> Maintainer Summit topic for someone to propose...)

--
Sincerely yours,
Mike.

2021-04-22 01:18:48

by Alexander Grund

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

> Below is the list that didn't do a simple "revert" that I need to look
> at. I was going to have my interns look into this, there's no need to
> bother busy maintainers with it unless you really want to, as I can't
> tell anyone what to work on :)

I'm not involved or affliated with the group or the kernel, but I'd like to make a suggestion:
Do not revert umn.edu patches unconditionally.
See below:

According to the paper:
> We submit the three patches using a randomGmail account to the Linux community andseek their feedback

So while their behaviour regarding this practice may have been bad, I'd give them the benefit of doubt that they didn't want to actually introduce
a bug.
I.e. what they wrote:

> we immediately notify themaintainers of the introduced UAF and request them to notgo ahead to apply the patch.
> At the same time, we point out the correct fixing of the bug and provide our correct patch.
> [...] All the UAF-introducing patches stayed only in the emailexchanges, without even becoming a Git commit in Linuxbranches

TLDR:
- The faulty patches were NOT from umn.edu accounts but from a gmail account
- Only the corrected patches should have made it to the branches

So while I would at least double-check that the last point is actually true, I believe reverting all umn.edu patches is wrong and actually (re-)introduces vulnerabilities or bugs which have been legitimately fixed (at least in good faith)
And especially if the reverts do not apply cleanly on the current HEADs I
think you might be wasting a lot of work/time, too.

And yes, this aftermath makes it even worse what they did and excluding them from future contributions may make sense.
But maybe reverting EVERYTHING is a bit to much here, especially if that doesn't even include the faulty stuff (assuming they are not plain lying in their paper, which I really doubt they would)

Alex


2021-04-22 01:22:06

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 11:35:00AM -0700, Weikeng Chen wrote:
>
> [1] I think the UMN IRB makes an incorrect assertion that the
> research is not human research, and that starts the entire problem
> and probably continues to be.

I think what we need to somehow establish is some norms about how
academic researchers engage with Open Source communities in general,
and the Linux Kernel community in particular.

To be fair, I don't know if Aditya Pakki was deliberately trying to
get nonsense patches in just to demonstrate that there is less review
for trivial patches, or whether he was creating a completely
incompetent, non-state-of-the-art static code analyzer, and was too
incompetent to hand check the patch to realize the results were
nonsense.

The big problem here is the lack of disclosure that the patch was
computer generated, using a new tool that might not be giving accurate
results, and that instead of diclosing this fact, submitting it as a
patch to be reviewed. Again, I don't know whether or not this was
submitted in bad faith --- but the point is, Aditya belongs to
research group which has previously submitted patches in bad faith,
without disclosure, and his supervising professor and UMN's IRB
doesn't see any problem with it. So it's a bit rich when Aditya seems
to be whining that we're not giving him the benefit of the doubt and
not assuming that his patches might have been submitted in good faith
--- when the only *responsible* thing to do is to assume that it is
sent in bad faith, given the past behaviour of his research group, and
the apparently lack of any kind of institutional controls at UMN
regarding this sort of thing.

Of course, UMN researchers could just start using fake e-mail
addresses, or start using personal gmail or yahoo or hotmail
addresses. (Hopefully at that point the ethics review boards at UMN
will be clueful enough to realize that maybe, just maybe, UMN
researchers have stepped over a line.)

However, your larger point is a fair one. We do need to do a better
job of reviewing patches, even "trivial" ones, and if that means that
we might need to be a bit more skeptical dealing with newbies who are
trying to get started, that's a price we will need to pay. Speaking
for myself, I've always tried to be highly skeptical about patches and
give them a thorough review. And I don't need to assume malice from
nation-state intelligence agencies; we're all human, and we all make
mistakes.

Cheers,

- Ted

2021-04-22 01:23:34

by Weikeng Chen

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

[This is the email that Theodore Ts'o replied to, but it fails to
reach the email server due to not using plain mode. Here I resent.]

(Note: this thread has become a hot Internet discussion on China's Twitter.)

I am a graduate student working in applied crypto, and CoI: I know one
of the authors of the S&P paper.
Some thoughts.

[1] I think the UMN IRB makes an incorrect assertion that the research
is not human research,
and that starts the entire problem and probably continues to be.

It clearly affects humans. I think UMN IRB lacks experience regarding
human experiments in CS research,
and should be informed that their decisions that this is not human
research are fundamentally wrong---
it misled the reviewers as well as misled the researchers.

---

[2] Banning UMN seems to be a temporary solution. I don't disagree.
But it still might not prevent such proof-of-concept efforts: one
could use a non-campus address.

It might be helpful to inform the PC chairs of major security
conferences, S&P, USENIX Security, CCS, and NDSS,
regarding the need to discourage software security papers from making
proofs-of-concept in the real world in wild
that may be hurtful, as well as concerns on the sufficiency of IRB
review---some IRB may lack experience for CS research.

Some conferences have been being more careful about this recently. For
example, NDSS accepts a paper on
a browser bug but attaches a statement saying that the PC has ethical concerns.
See: "Tales of Favicons and Caches: Persistent Tracking in Modern
Browsers", NDSS '21

---

[3] Let us not forget that the author is using their real campus
address and is open to such pressure.
Thus, I think the authors, as students and researchers, have no bad faith;
but they are misled that this experimental procedure is acceptable,
which is not.

Sorry for jumping in...

Weikeng

2021-04-22 01:26:43

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Tue, Apr 06, 2021 at 07:16:56PM -0500, Aditya Pakki wrote:
> In gss_pipe_destroy_msg(), in case of error in msg, gss_release_msg
> deletes gss_msg. The patch adds a check to avoid a potential double
> free.
>
> Signed-off-by: Aditya Pakki <[email protected]>
> ---
> net/sunrpc/auth_gss/auth_gss.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> index 5f42aa5fc612..eb52eebb3923 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -848,7 +848,8 @@ gss_pipe_destroy_msg(struct rpc_pipe_msg *msg)
> warn_gssd();
> gss_release_msg(gss_msg);
> }
> - gss_release_msg(gss_msg);
> + if (gss_msg)
> + gss_release_msg(gss_msg);

I know I am adding to the noise here, but it has to be said:
gss_msg is assigned with
struct gss_upcall_msg *gss_msg = container_of(msg, struct gss_upcall_msg, msg);
and thus never NULL.

Guenter

> }
>
> static void gss_pipe_dentry_destroy(struct dentry *dir,
> --
> 2.25.1
>

2021-04-22 03:58:11

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 08:34:59PM +0300, Mike Rapoport wrote:
> On Wed, Apr 21, 2021 at 11:48:46AM -0400, Theodore Ts'o wrote:
> > On Wed, Apr 21, 2021 at 05:15:26PM +0300, Leon Romanovsky wrote:
> > > > This thread is the first I'm hearing about this. I wonder if there is
> > > > a good way of alerting the entire kernel community (including those
> > > > only subscribed to subsystem mailing lists) about what's going on? It
> > > > seems like useful information to have to push back against these
> > > > patches.
>
> Heh, I've got this information from google news feed on my phone :)
>
> > > IMHO, kernel users ML is good enough for that.
> >
> > The problem is that LKML is too high traffic for a lot of people to
> > want to follow.
>
> I think Leon meant kernel.org users ML ([email protected]). Along with
> ksummut-discuss it'll reach most maintainers, IMHO.

Exactly.

Thanks

>
> > There are some people who have used the kernel summit discuss list
> > (previously [email protected], now
> > [email protected]) as a place where most maintainers tend to be
> > subscribed, although that's not really a guarantee, either. (Speaking
> > of which, how to handle groups who submit patches in bad faith a good
> > Maintainer Summit topic for someone to propose...)
>
> --
> Sincerely yours,
> Mike.

2021-04-22 07:50:39

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 03:49:51PM -0400, Theodore Ts'o wrote:
>
> Of course, UMN researchers could just start using fake e-mail
> addresses, or start using personal gmail or yahoo or hotmail
> addresses. (Hopefully at that point the ethics review boards at UMN
> will be clueful enough to realize that maybe, just maybe, UMN
> researchers have stepped over a line.)
>

They are actually already doing (or did) that -- see page 9 of their paper
(https://github.com/QiushiWu/QiushiWu.github.io/blob/main/papers/OpenSourceInsecurity.pdf)
where they say they use "a random email account" to send patches.

I think that (two of?) the accounts they used were
James Bond <[email protected]>
(https://lore.kernel.org/lkml/?q=jameslouisebond%40gmail.com) and
George Acosta <[email protected]>
(https://lore.kernel.org/lkml/?q=acostag.ubuntu%40gmail.com). Most of their
patches match up very closely with commits they described in their paper:

Figure 9 = https://lore.kernel.org/lkml/[email protected]/
Figure 10 = https://lore.kernel.org/lkml/[email protected]/
Figure 11 = https://lore.kernel.org/lkml/[email protected]/

Unfortunately they obfuscated the code in their paper for some bizarre reason
and don't provide a proper list, so it's hard to know for sure though. And
there could be more patches elsewhere.

Note that the "Figure 11" patch was actually accepted and is in mainline.
However it's not actually a bug; apparently they didn't realize that.

- Eric

2021-04-22 08:12:04

by Sudip Mukherjee

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

Hi Greg,

On Wed, Apr 21, 2021 at 11:21 AM Greg KH <[email protected]> wrote:
>
> On Wed, Apr 21, 2021 at 11:07:11AM +0100, Sudip Mukherjee wrote:
> > Hi Greg,
> >
> > On Wed, Apr 21, 2021 at 6:44 AM Greg KH <[email protected]> wrote:
> > >
> > > On Wed, Apr 21, 2021 at 08:10:25AM +0300, Leon Romanovsky wrote:
> > > > On Tue, Apr 20, 2021 at 01:10:08PM -0400, J. Bruce Fields wrote:
> > > > > On Tue, Apr 20, 2021 at 09:15:23AM +0200, Greg KH wrote:
> > > > > > If you look at the code, this is impossible to have happen.
> > > > > >
> >
> > <snip>
> >
> > > > They introduce kernel bugs on purpose. Yesterday, I took a look on 4
> > > > accepted patches from Aditya and 3 of them added various severity security
> > > > "holes".
> > >
> > > All contributions by this group of people need to be reverted, if they
> > > have not been done so already, as what they are doing is intentional
> > > malicious behavior and is not acceptable and totally unethical. I'll
> > > look at it after lunch unless someone else wants to do it...
> >
> > A lot of these have already reached the stable trees. I can send you
> > revert patches for stable by the end of today (if your scripts have
> > not already done it).
>
> Yes, if you have a list of these that are already in the stable trees,
> that would be great to have revert patches, it would save me the extra
> effort these mess is causing us to have to do...

The patch series for all the stable branches should be with you now.

But for others:
https://lore.kernel.org/stable/YIEVGXEoeizx6O1p@debian/ for v5.11.y
and other branches are sent as a reply to that mail.


--
Regards
Sudip

2021-04-22 08:27:52

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Thu, Apr 22, 2021 at 09:10:53AM +0100, Sudip Mukherjee wrote:
> Hi Greg,
>
> On Wed, Apr 21, 2021 at 11:21 AM Greg KH <[email protected]> wrote:
> >
> > On Wed, Apr 21, 2021 at 11:07:11AM +0100, Sudip Mukherjee wrote:
> > > Hi Greg,
> > >
> > > On Wed, Apr 21, 2021 at 6:44 AM Greg KH <[email protected]> wrote:
> > > >
> > > > On Wed, Apr 21, 2021 at 08:10:25AM +0300, Leon Romanovsky wrote:
> > > > > On Tue, Apr 20, 2021 at 01:10:08PM -0400, J. Bruce Fields wrote:
> > > > > > On Tue, Apr 20, 2021 at 09:15:23AM +0200, Greg KH wrote:
> > > > > > > If you look at the code, this is impossible to have happen.
> > > > > > >
> > >
> > > <snip>
> > >
> > > > > They introduce kernel bugs on purpose. Yesterday, I took a look on 4
> > > > > accepted patches from Aditya and 3 of them added various severity security
> > > > > "holes".
> > > >
> > > > All contributions by this group of people need to be reverted, if they
> > > > have not been done so already, as what they are doing is intentional
> > > > malicious behavior and is not acceptable and totally unethical. I'll
> > > > look at it after lunch unless someone else wants to do it...
> > >
> > > A lot of these have already reached the stable trees. I can send you
> > > revert patches for stable by the end of today (if your scripts have
> > > not already done it).
> >
> > Yes, if you have a list of these that are already in the stable trees,
> > that would be great to have revert patches, it would save me the extra
> > effort these mess is causing us to have to do...
>
> The patch series for all the stable branches should be with you now.
>
> But for others:
> https://lore.kernel.org/stable/YIEVGXEoeizx6O1p@debian/ for v5.11.y
> and other branches are sent as a reply to that mail.

Thank you, I now have them. I will be looking at them when I get the
chance, and comparing them to what I end up getting merged into
5.13-rc1.

greg k-h

2021-04-22 19:40:28

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Wed, Apr 21, 2021 at 09:56:37AM -0400, J. Bruce Fields wrote:
> On Wed, Apr 21, 2021 at 04:49:31PM +0300, Leon Romanovsky wrote:
> > If you want to see another accepted patch that is already part of
> > stable@, you are invited to take a look on this patch that has "built-in bug":
> > 8e949363f017 ("net: mlx5: Add a missing check on idr_find, free buf")
>
> Interesting, thanks.

Though looking at it now, I'm not actually seeing the bug--probably I'm
overlooking something obvious.

--b.

2021-04-23 17:26:16

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Thu, Apr 22, 2021 at 03:39:50PM -0400, J. Bruce Fields wrote:
> On Wed, Apr 21, 2021 at 09:56:37AM -0400, J. Bruce Fields wrote:
> > On Wed, Apr 21, 2021 at 04:49:31PM +0300, Leon Romanovsky wrote:
> > > If you want to see another accepted patch that is already part of
> > > stable@, you are invited to take a look on this patch that has "built-in bug":
> > > 8e949363f017 ("net: mlx5: Add a missing check on idr_find, free buf")
> >
> > Interesting, thanks.
>
> Though looking at it now, I'm not actually seeing the bug--probably I'm
> overlooking something obvious.

It was fixed in commit 31634bf5dcc4 ("net/mlx5: FPGA, tls, hold rcu read lock a bit longer")

Thanks

>
> --b.

2021-04-23 18:08:39

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Fri, Apr 23, 2021 at 08:25:28PM +0300, Leon Romanovsky wrote:
> On Thu, Apr 22, 2021 at 03:39:50PM -0400, J. Bruce Fields wrote:
> > On Wed, Apr 21, 2021 at 09:56:37AM -0400, J. Bruce Fields wrote:
> > > On Wed, Apr 21, 2021 at 04:49:31PM +0300, Leon Romanovsky wrote:
> > > > If you want to see another accepted patch that is already part of
> > > > stable@, you are invited to take a look on this patch that has "built-in bug":
> > > > 8e949363f017 ("net: mlx5: Add a missing check on idr_find, free buf")
> > >
> > > Interesting, thanks.
> >
> > Though looking at it now, I'm not actually seeing the bug--probably I'm
> > overlooking something obvious.
>
> It was fixed in commit 31634bf5dcc4 ("net/mlx5: FPGA, tls, hold rcu read lock a bit longer")

So is the "Fixes:" line on that commit wrong? It claims the bug was
introduced by an earlier commit, ab412e1dd7db ("net/mlx5: Accel, add TLS
rx offload routines").

Looks like Aditya Pakki's commit may have widened the race a little, but
I find it a little hard to fault him for that.

--b.

2021-04-23 19:30:38

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Fri, Apr 23, 2021 at 02:07:27PM -0400, J. Bruce Fields wrote:
> On Fri, Apr 23, 2021 at 08:25:28PM +0300, Leon Romanovsky wrote:
> > On Thu, Apr 22, 2021 at 03:39:50PM -0400, J. Bruce Fields wrote:
> > > On Wed, Apr 21, 2021 at 09:56:37AM -0400, J. Bruce Fields wrote:
> > > > On Wed, Apr 21, 2021 at 04:49:31PM +0300, Leon Romanovsky wrote:
> > > > > If you want to see another accepted patch that is already part of
> > > > > stable@, you are invited to take a look on this patch that has "built-in bug":
> > > > > 8e949363f017 ("net: mlx5: Add a missing check on idr_find, free buf")
> > > >
> > > > Interesting, thanks.
> > >
> > > Though looking at it now, I'm not actually seeing the bug--probably I'm
> > > overlooking something obvious.
> >
> > It was fixed in commit 31634bf5dcc4 ("net/mlx5: FPGA, tls, hold rcu read lock a bit longer")
>
> So is the "Fixes:" line on that commit wrong? It claims the bug was
> introduced by an earlier commit, ab412e1dd7db ("net/mlx5: Accel, add TLS
> rx offload routines").

Yes, I think that Fixes line is misleading.

>
> Looks like Aditya Pakki's commit may have widened the race a little, but
> I find it a little hard to fault him for that.

We can argue about severity of this bug, but the whole paper talks about
introduction of UAF bugs unnoticed.

Thanks

>
> --b.

2021-04-23 21:49:29

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

Have umn addresses been blocked from posting to kernel lists?

Anyway:

On Fri, Apr 23, 2021 at 10:29:52PM +0300, Leon Romanovsky wrote:
> On Fri, Apr 23, 2021 at 02:07:27PM -0400, J. Bruce Fields wrote:
> > On Fri, Apr 23, 2021 at 08:25:28PM +0300, Leon Romanovsky wrote:
> > > On Thu, Apr 22, 2021 at 03:39:50PM -0400, J. Bruce Fields wrote:
> > > > On Wed, Apr 21, 2021 at 09:56:37AM -0400, J. Bruce Fields wrote:
> > > > > On Wed, Apr 21, 2021 at 04:49:31PM +0300, Leon Romanovsky wrote:
> > > > > > If you want to see another accepted patch that is already part of
> > > > > > stable@, you are invited to take a look on this patch that has "built-in bug":
> > > > > > 8e949363f017 ("net: mlx5: Add a missing check on idr_find, free buf")
> > > > >
> > > > > Interesting, thanks.
> > > >
> > > > Though looking at it now, I'm not actually seeing the bug--probably I'm
> > > > overlooking something obvious.
> > >
> > > It was fixed in commit 31634bf5dcc4 ("net/mlx5: FPGA, tls, hold rcu read lock a bit longer")
> >
> > So is the "Fixes:" line on that commit wrong? It claims the bug was
> > introduced by an earlier commit, ab412e1dd7db ("net/mlx5: Accel, add TLS
> > rx offload routines").
>
> Yes, I think that Fixes line is misleading.
>
> >
> > Looks like Aditya Pakki's commit may have widened the race a little, but
> > I find it a little hard to fault him for that.
>
> We can argue about severity of this bug, but the whole paper talks about
> introduction of UAF bugs unnoticed.

Aditya Pakki points out in private mail that this patch is part of the
work described in this paper:

https://www-users.cs.umn.edu/~kjlu/papers/crix.pdf

(See the list of patches in the appendix.)

I mean, sure, I suppose they could have created that whole second line
of research just as a cover to submit malicious patches, but I think
we're running pretty hard into Occam's Razor at that point.

--b.

2021-04-24 07:22:15

by Leon Romanovsky

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Fri, Apr 23, 2021 at 05:48:50PM -0400, J. Bruce Fields wrote:
> Have umn addresses been blocked from posting to kernel lists?

It is very unlikely.

>
> Anyway:
>
> On Fri, Apr 23, 2021 at 10:29:52PM +0300, Leon Romanovsky wrote:
> > On Fri, Apr 23, 2021 at 02:07:27PM -0400, J. Bruce Fields wrote:
> > > On Fri, Apr 23, 2021 at 08:25:28PM +0300, Leon Romanovsky wrote:
> > > > On Thu, Apr 22, 2021 at 03:39:50PM -0400, J. Bruce Fields wrote:
> > > > > On Wed, Apr 21, 2021 at 09:56:37AM -0400, J. Bruce Fields wrote:
> > > > > > On Wed, Apr 21, 2021 at 04:49:31PM +0300, Leon Romanovsky wrote:
> > > > > > > If you want to see another accepted patch that is already part of
> > > > > > > stable@, you are invited to take a look on this patch that has "built-in bug":
> > > > > > > 8e949363f017 ("net: mlx5: Add a missing check on idr_find, free buf")
> > > > > >
> > > > > > Interesting, thanks.
> > > > >
> > > > > Though looking at it now, I'm not actually seeing the bug--probably I'm
> > > > > overlooking something obvious.
> > > >
> > > > It was fixed in commit 31634bf5dcc4 ("net/mlx5: FPGA, tls, hold rcu read lock a bit longer")
> > >
> > > So is the "Fixes:" line on that commit wrong? It claims the bug was
> > > introduced by an earlier commit, ab412e1dd7db ("net/mlx5: Accel, add TLS
> > > rx offload routines").
> >
> > Yes, I think that Fixes line is misleading.
> >
> > >
> > > Looks like Aditya Pakki's commit may have widened the race a little, but
> > > I find it a little hard to fault him for that.
> >
> > We can argue about severity of this bug, but the whole paper talks about
> > introduction of UAF bugs unnoticed.
>
> Aditya Pakki points out in private mail that this patch is part of the
> work described in this paper:
>
> https://www-users.cs.umn.edu/~kjlu/papers/crix.pdf
>
> (See the list of patches in the appendix.)
>
> I mean, sure, I suppose they could have created that whole second line
> of research just as a cover to submit malicious patches, but I think
> we're running pretty hard into Occam's Razor at that point.

Let's not speculate here.

The lack of trust, due to unethical research that was done by UMN researchers,
amount of bugs already introduced by @umn, and multiple attempts to repeat the
same pattern (see Al Viro responses on patches like this SUNRPC patch) is enough
to stop waste our time.

Thanks

>
> --b.

2021-04-24 18:35:51

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Fri, Apr 23, 2021 at 05:48:50PM -0400, J. Bruce Fields wrote:
> Have umn addresses been blocked from posting to kernel lists?

I don't see davem ever doing anything of that sort. I've no information
about what has really happened, but "Uni lawyers and/or HR telling them
to stop making anything that might be considered public statements" sounds
much more plausible...

Again, it's only a speculation and it might very well have been something
else, but out of those two variants... I'd put high odds on the latter vs
the former.

2021-04-24 21:36:30

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Sat, Apr 24, 2021 at 06:34:45PM +0000, Al Viro wrote:
> On Fri, Apr 23, 2021 at 05:48:50PM -0400, J. Bruce Fields wrote:
> > Have umn addresses been blocked from posting to kernel lists?
>
> I don't see davem ever doing anything of that sort. I've no information
> about what has really happened, but "Uni lawyers and/or HR telling them
> to stop making anything that might be considered public statements" sounds
> much more plausible...
>
> Again, it's only a speculation and it might very well have been something
> else, but out of those two variants... I'd put high odds on the latter vs
> the former.

From private email: "Our UMN emails addresses are already banned from
the mailing list."

Also, I didn't get a copy of

CA+EnHHSw4X+ubOUNYP2zXNpu70G74NN1Sct2Zin6pRgq--TqhA@mail.gmail.com

through vger for some reason, and it didn't make it to lore.kernel.org either.

In Greg's revert thread, Kangjie Lu's messages are also missing from the
archives:

https://lore.kernel.org/lkml/[email protected]/

??

--b.

2021-04-25 06:30:25

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Sat, Apr 24, 2021 at 08:41:27PM -0400, Theodore Ts'o wrote:
> On Sat, Apr 24, 2021 at 05:34:54PM -0400, J. Bruce Fields wrote:
> > In Greg's revert thread, Kangjie Lu's messages are also missing from the
> > archives:
> >
> > https://lore.kernel.org/lkml/[email protected]/
> >
>
> I'm going to guess it's one of two things. The first is that they are
> sending mail messages with HTML which is getting bounced; the other
> possibility is that some of the messages were sent only to Greg, and
> he added the mailing list back to the cc.
>
> So for exampple, message-id
> CA+EnHHSw4X+ubOUNYP2zXNpu70G74NN1Sct2Zin6pRgq--TqhA@mail.gmail.com
> isn't in lore, but Greg's reply:
>
> https://lore.kernel.org/linux-nfs/YH%2FfM%[email protected]/
>
> can be found in lore.kernel.org was presumably because the message
> where Aditya accused "wild accusations bordering on slander" and his
> claim that his patches were the fault of a "new static code analyzer"
> was sent only to Greg? Either that, or it was bounced because he sent
> it from gmail without suppressing HTML.

I did not "add back" the mailing list, it looks like they sent email in
html format which prevented it from hitting the public lists. I have
the originals sent to me that shows the author intended it to be public.

thanks,

greg k-h

2021-04-26 13:48:11

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] SUNRPC: Add a check for gss_release_msg

On Mon, Apr 26, 2021 at 09:36:05AM -0400, J. Bruce Fields wrote:
> On Sun, Apr 25, 2021 at 08:29:53AM +0200, Greg KH wrote:
> > On Sat, Apr 24, 2021 at 08:41:27PM -0400, Theodore Ts'o wrote:
> > > On Sat, Apr 24, 2021 at 05:34:54PM -0400, J. Bruce Fields wrote:
> > > > In Greg's revert thread, Kangjie Lu's messages are also missing from the
> > > > archives:
> > > >
> > > > https://lore.kernel.org/lkml/[email protected]/
> > > >
> > >
> > > I'm going to guess it's one of two things. The first is that they are
> > > sending mail messages with HTML which is getting bounced; the other
> > > possibility is that some of the messages were sent only to Greg, and
> > > he added the mailing list back to the cc.
> > >
> > > So for exampple, message-id
> > > CA+EnHHSw4X+ubOUNYP2zXNpu70G74NN1Sct2Zin6pRgq--TqhA@mail.gmail.com
> > > isn't in lore, but Greg's reply:
> > >
> > > https://lore.kernel.org/linux-nfs/YH%2FfM%[email protected]/
> > >
> > > can be found in lore.kernel.org was presumably because the message
> > > where Aditya accused "wild accusations bordering on slander" and his
> > > claim that his patches were the fault of a "new static code analyzer"
> > > was sent only to Greg? Either that, or it was bounced because he sent
> > > it from gmail without suppressing HTML.
> >
> > I did not "add back" the mailing list, it looks like they sent email in
> > html format which prevented it from hitting the public lists. I have
> > the originals sent to me that shows the author intended it to be public.
>
> Yes, the list cc's are all on there.
>
> It's multipart/alternative with equivalent plain text and html parts,
> which appears to be gmail's default behavior. Is that really rejected
> by default?

Hah, when I sent that mail I quoted parts of the message including the
Content-Type headers delineating the parts and got an immediate bounce
saying "The message contains HTML subpart, therefore we consider it SPAM
or Outlook Virus. TEXT/PLAIN is accepted".

Which seems perfectly clear. OK, sorry for the noise!

--b.