Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

[Michal Hocko]

CVE-2024-35881 to revert f341055b10bd ("drm/amd/display: Send DTBCLK
disable message on first commit") by 3a6a32b31a11 ("Revert
"drm/amd/display: Send DTBCLK disable message on first commit"") has
been filed as well.

Is this really intentional? Should both be rejected?

On Sun 19-05-24 10:35:20, Greg KH wrote:
> Description
> ===========
> 
> In the Linux kernel, the following vulnerability has been resolved:
> 
> drm/amd/display: Send DTBCLK disable message on first commit
> 
> [Why]
> Previous patch to allow DTBCLK disable didn't address boot case. Driver
> thinks DTBCLK is disabled by default, so we don't send disable message to
> PMFW. DTBCLK is then enabled at idle desktop on boot, burning power.
> 
> [How]
> Set dtbclk_en to true on boot so that disable message is sent during first
> commit.
> 
> The Linux kernel CVE team has assigned CVE-2024-35906 to this issue.
> 
> 
> Affected and fixed versions
> ===========================
> 
> 	Fixed in 6.8.5 with commit 0dab75b433ed
> 	Fixed in 6.9 with commit f341055b10bd
> 
> Please see https://www.kernel.org for a full list of currently supported
> kernel versions by the kernel community.
> 
> Unaffected versions might change over time as fixes are backported to
> older supported kernel versions.  The official CVE entry at
> 	https://cve.org/CVERecord/?id=CVE-2024-35906
> will be updated if fixes are backported, please check that for the most
> up to date information about this issue.
> 
> 
> Affected files
> ==============
> 
> The file(s) affected by this issue are:
> 	drivers/gpu/drm/amd/display/dc/clk_mgr/dcn35/dcn35_clk_mgr.c
> 
> 
> Mitigation
> ==========
> 
> The Linux kernel CVE team recommends that you update to the latest
> stable kernel version for this, and many other bugfixes.  Individual
> changes are never tested alone, but rather are part of a larger kernel
> release.  Cherry-picking individual commits is not recommended or
> supported by the Linux kernel community at all.  If however, updating to
> the latest release is impossible, the individual changes to resolve this
> issue can be found at these commits:
> 	https://git.kernel.org/stable/c/0dab75b433ed2480d57ae4f8f725186a46223e42
> 	https://git.kernel.org/stable/c/f341055b10bd8be55c3c995dff5f770b236b8ca9

-- 
Michal Hocko
SUSE Labs

Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

[Greg Kroah-Hartman]

On Tue, May 21, 2024 at 10:28:41AM +0200, Michal Hocko wrote:
> CVE-2024-35881 to revert f341055b10bd ("drm/amd/display: Send DTBCLK
> disable message on first commit") by 3a6a32b31a11 ("Revert
> "drm/amd/display: Send DTBCLK disable message on first commit"") has
> been filed as well.
> 
> Is this really intentional? Should both be rejected?

I don't think so as we had releases with the original commit in it,
which was buggy so then the second one was needed.  So if you only took
the first fix, you have a problem, and need the second one.  If you take
both, all is good.  If you took neither, also all is good.  So be safe
and take both :)

thanks,

greg k-h

Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

[Michal Hocko]

On Tue 21-05-24 16:39:51, Greg KH wrote:
> On Tue, May 21, 2024 at 10:28:41AM +0200, Michal Hocko wrote:
> > CVE-2024-35881 to revert f341055b10bd ("drm/amd/display: Send DTBCLK
> > disable message on first commit") by 3a6a32b31a11 ("Revert
> > "drm/amd/display: Send DTBCLK disable message on first commit"") has
> > been filed as well.
> > 
> > Is this really intentional? Should both be rejected?
> 
> I don't think so as we had releases with the original commit in it,

I do not think so. Looking at stable kernel branches:
$ git describe-ver 0dab75b433ed2480d57ae4f8f725186a46223e42
v6.8.5~88
$ git describe-ver d6d5622f64f3e07620683d61c880f57965fe1b48
v6.8.5~239

Both of them were released in 6.9-rc1 in Linus tree. I do not see them
in any other stable trees. Neither of them is even marked for stable and
they seemed to be merged only because of (stable tree) 7ea8a0e12088eb0c
which has Stable-dep-of: f341055b10bd ("drm/amd/display: Send DTBCLK
disable message on first commit"). Btw note that 7ea8a0e12088eb0c is not
marked for stable, nor I see anybody requesting that on lore.
Stable rulez!

Let's put aside whether f341055b10bd should get a CVE, we have clearly a
different view on that but looking at the vulns.git tree both CVEs have
been assigned together
$ git log ./2024/CVE-2024-35906.sha1 ./2024/CVE-2024-35881.sha1
commit a6191f0053349c3234f690316d6511e97927f28f
Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date:   Sun May 19 10:35:32 2024 +0200

    some 6.8.5 cves assigned

    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

which to me indicates that both CVEs were assigned by a script
without a proper review which is really unfortunate.

Please keep in mind that there are actual consumers of these CVEs and
you are burning their time evaluating these noops. A waste of time, if
you ask me, and not something that could be just neglected considering
how many CVEs you are producing.
-- 
Michal Hocko
SUSE Labs

Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

[Greg Kroah-Hartman]

On Tue, May 21, 2024 at 06:51:28PM +0200, Michal Hocko wrote:
> On Tue 21-05-24 16:39:51, Greg KH wrote:
> > On Tue, May 21, 2024 at 10:28:41AM +0200, Michal Hocko wrote:
> > > CVE-2024-35881 to revert f341055b10bd ("drm/amd/display: Send DTBCLK
> > > disable message on first commit") by 3a6a32b31a11 ("Revert
> > > "drm/amd/display: Send DTBCLK disable message on first commit"") has
> > > been filed as well.
> > > 
> > > Is this really intentional? Should both be rejected?
> > 
> > I don't think so as we had releases with the original commit in it,
> 
> I do not think so. Looking at stable kernel branches:
> $ git describe-ver 0dab75b433ed2480d57ae4f8f725186a46223e42
> v6.8.5~88
> $ git describe-ver d6d5622f64f3e07620683d61c880f57965fe1b48
> v6.8.5~239

Ah, missed that, sorry, was thinking about a different set of reverts
recently assigned.

> Both of them were released in 6.9-rc1 in Linus tree. I do not see them
> in any other stable trees. Neither of them is even marked for stable and
> they seemed to be merged only because of (stable tree) 7ea8a0e12088eb0c
> which has Stable-dep-of: f341055b10bd ("drm/amd/display: Send DTBCLK
> disable message on first commit"). Btw note that 7ea8a0e12088eb0c is not
> marked for stable, nor I see anybody requesting that on lore.
> Stable rulez!
> 
> Let's put aside whether f341055b10bd should get a CVE, we have clearly a
> different view on that but looking at the vulns.git tree both CVEs have
> been assigned together
> $ git log ./2024/CVE-2024-35906.sha1 ./2024/CVE-2024-35881.sha1
> commit a6191f0053349c3234f690316d6511e97927f28f
> Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Date:   Sun May 19 10:35:32 2024 +0200
> 
>     some 6.8.5 cves assigned
> 
>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> which to me indicates that both CVEs were assigned by a script
> without a proper review which is really unfortunate.

Note, we have checks for many of these things, where we have an affected
kernel and a fix in the same release, and we do NOT assign a CVE for
those types of things.  This is the first time that I know of where we
have had this happen where the bug and revert was in the same release,
hard part here was that the revert didn't have a Fixes: tag, if it did,
we would have caught it.

> Please keep in mind that there are actual consumers of these CVEs and
> you are burning their time evaluating these noops. A waste of time, if
> you ask me, and not something that could be just neglected considering
> how many CVEs you are producing.

We will have a small % of issues that we miss and mess up like this,
that's just because we are all human.  Your help in reviewing these is
greatly appreciated.  Right now I still feel we are not catching all
that we should be catching to mark as a CVE, so we are open for anyone
to help us out with reviews.  Luckily we have a few more people helping
now as well, which is great.

And really, in the end, if you have a "CVE and fix for CVE" in the same
release, applying both doesn't hurt anyone, so this is a "fail secure"
mode for everyone, right?

thanks,

greg k-h

Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

[Michal Hocko]

On Tue 21-05-24 19:03:58, Greg KH wrote:
> On Tue, May 21, 2024 at 06:51:28PM +0200, Michal Hocko wrote:
[...]
> And really, in the end, if you have a "CVE and fix for CVE" in the same
> release, applying both doesn't hurt anyone, so this is a "fail secure"
> mode for everyone, right?

Look Greg, we have been through this discussion at several occasions and
I do not want to repeat that again. Stable tree users probably do not
care because they are getting all these patches, including regressions
and patches they do not need or even want, anyway. They are getting what
they are _paying_ for. Marking them CVE doesn't make any difference. But
stable tree backporting model is simply not a good fit for _many_ users.

Now, for $reasons, people _do_ care about CVEs and that is why there is
an engineering cost on downstreams to review them. Exactly because
applying all of them blindly is a _risk_. Exactly because the stable
backporting model/policy and CVE assigning policy is simply incompatible
with the stability/correctness/performance/$other requirements. 

I completely do get why you do not care about that downstream
engineering cost but generating bogus CVEs on top of a huge pile of
dubious ones is less than useful, don't you think?

Seriously, we can disagree whether something is a security threat that
is worth marking by a CVE. But making the CVE generation process mostly
unattended script driven process without any _serious_ review in place
is burning a lot of man power that could be used in a much more
productive way. This is not the way how to convince people to use stable
kernels.

Bye
-- 
Michal Hocko
SUSE Labs

Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

[Greg Kroah-Hartman]

On Tue, May 21, 2024 at 07:56:54PM +0200, Michal Hocko wrote:
> On Tue 21-05-24 19:03:58, Greg KH wrote:
> > On Tue, May 21, 2024 at 06:51:28PM +0200, Michal Hocko wrote:
> [...]
> > And really, in the end, if you have a "CVE and fix for CVE" in the same
> > release, applying both doesn't hurt anyone, so this is a "fail secure"
> > mode for everyone, right?
> 
> Look Greg, we have been through this discussion at several occasions and
> I do not want to repeat that again. Stable tree users probably do not
> care because they are getting all these patches, including regressions
> and patches they do not need or even want, anyway. They are getting what
> they are _paying_ for. Marking them CVE doesn't make any difference. But
> stable tree backporting model is simply not a good fit for _many_ users.

I understand you feel this way, but I can still disagree that it results
in a secure system for users.  But let's ignore that for now...

> Now, for $reasons, people _do_ care about CVEs and that is why there is
> an engineering cost on downstreams to review them. Exactly because
> applying all of them blindly is a _risk_. Exactly because the stable
> backporting model/policy and CVE assigning policy is simply incompatible
> with the stability/correctness/performance/$other requirements. 

That's your business decision, NOT mine.

> I completely do get why you do not care about that downstream
> engineering cost but generating bogus CVEs on top of a huge pile of
> dubious ones is less than useful, don't you think?

How is this a "bogus" CVE on their own?  Both issues involved here
classify as such, the only issue is that the fix/revert showed up in the
same release.  And yes, as such, it does not qualify under the CVE rules
as they want to see the bug in a release, and so our tools normally
catch this type of thing.

BUT if you have a model of "I cherry-pick what I want", you WILL miss
things like this that could be considered serious.  So note that by us
not classifying stuff like this as a CVE, YOU run the risk of missing
this and maybe ending up with a bug in your system (i.e. you took the
bug, but not the fix.)

That's your risk, yes, and your business decision to do so, the commits
here in question being marked as a CVE make your life easier, not
harder, so while I will go revoke these, realize this means that you now
need to do more work, not less, for the future.  Sorry about that.

> Seriously, we can disagree whether something is a security threat that
> is worth marking by a CVE. But making the CVE generation process mostly
> unattended script driven process without any _serious_ review in place
> is burning a lot of man power that could be used in a much more
> productive way. This is not the way how to convince people to use stable
> kernels.

To think that any of this is an "unattended script without any _serious_
review" is slandering all of the people who put in their free time to do
this work for you and the community.  This is ANYTHING BUT an unattended
script.  Personally I have "wasted" weeks, if not months, of development
time I could have spent in review of kernel patches, or writing kernel
fixes, in order to get this all working properly, having meetings, going
through "CVE CNA training", writing bash scripts, writing tests for bash
scripts, fixing bash scripts based on horrible data in free-form
changelog commits, and most importantly, reviewing 100+ commits a week
yet-again.

Then there is the other developers here doing this work, it's not just
me.  For every CVE you see assigned, it has been reviewed and agreed
apon by at LEAST 2 different independent people.  Usually 3.  That is
anything but "unattended".  If you wanted "unattended", I would just
crank out a CVE for every stable commit that is added to the tree, but
that's not what is happening.  We are manually reviewing every commit to
see if it matches up with the CVE vulnerability guidelines and
classifying it as such.  To tell people that the work they do is not
even happening is rude.

Also, this is work ostensibly that you are also already doing for your
day-job, right?  Surely you also are reviewing each and every commit
that ends up in the stable kernels at the very least to evaluate if they
are security or just bug fixes for your corporate offerings, right?  So
you already have this work done, why does it matter if a CVE is assigned
to a commit or not, you all already know if it is relevent or not for
your kernels before that assignment happens, so you can trivially just
match up the ids to see if perhaps you missed something or not, right?

So along those lines, why not help us out and provide us with a list of
commits that you feel _should_ be assigned CVEs, and an annotated list
of those that you feel _should not_ be assigned, like we are currently
doing today as part of our review process?  We already have external
people helping us out already, sending us patches for our review lists
and providing a fourth and fifth set of review eyes on things for where
we miss stuff, OR where we get things wrong (hey, we are human, we will
do both as this is NOT automated.)

I appreciate your reviews so far of "hey, this shouldn't really be a
CVE, right?"  That's a lot of help and is making things better, but to
insinuate that somehow we don't know what the hell we are doing, or that
we are doing nothing other than a simple "assign everything!" process
here is extremely insulting to me and to the other developers here
working on this.

So again, I welcome your help, and I understand your frustration that
somehow you feel we are now making you do more work than before, but if
that really is the case, then you really weren't actually looking at the
patch stream of fixes that you should have been looking at, and I doubt
that's something you want to admit to in public :)

thanks,

greg k-h

Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

[Michal Hocko]

On Wed 22-05-24 05:57:38, Greg KH wrote:
[...]
> > I completely do get why you do not care about that downstream
> > engineering cost but generating bogus CVEs on top of a huge pile of
> > dubious ones is less than useful, don't you think?
> 
> How is this a "bogus" CVE on their own?

I suspect you haven't looked at those commits. This is a boot time
suboptimal HW configuration. There is no way any user can exploit that I
can see. Not to mention it cases system boot hangs!

[...]
> > Seriously, we can disagree whether something is a security threat that
> > is worth marking by a CVE. But making the CVE generation process mostly
> > unattended script driven process without any _serious_ review in place
> > is burning a lot of man power that could be used in a much more
> > productive way. This is not the way how to convince people to use stable
> > kernels.
> 
> To think that any of this is an "unattended script without any _serious_
> review" is slandering all of the people who put in their free time to do
> this work for you and the community.  This is ANYTHING BUT an unattended
> script.

This is a perception one can easily draw by watching the stream of
incoming flow. Sorry if that is not the case but there is about zero
transparency about the process except for Documentation/process/cve.rst
and let me quote
"
As part of the normal stable release process, kernel changes that are
potentially security issues are identified by the developers responsible
for CVE number assignments and have CVE numbers automatically assigned
to them.
"

There is no mention about criteria, review process. Who is involve in
the assignment and who is doing the review. The vulns.git tree doesn't
contain Sign-off-by of those involved parties except for the submitter.

> Also, this is work ostensibly that you are also already doing for your
> day-job, right?

We, like stable trees, rely on Fixes tag and those (including other
commits that might be this tag) are reviewed by domain experts.

[...]

> I appreciate your reviews so far of "hey, this shouldn't really be a
> CVE, right?"  That's a lot of help and is making things better, but to
> insinuate that somehow we don't know what the hell we are doing, or that
> we are doing nothing other than a simple "assign everything!" process
> here is extremely insulting to me and to the other developers here
> working on this.

I am sorry if you feel that way but I haven't said anything like that.

I have raised a concern based on observed CVE flow that the current
process is automated without a proper review as I can see very dubious
CVEs being assigned (splats resembling oops/warnings coming from lockdep,
data_race fixes as they resemble race condition fixes, READ_ONCE fixes
which do not fix anything discussed in other thread and others).

I have tried to dispute quite some but quickly learned that many of them
have been dismissed because "no usecases are assumed". It is a very
broad category that could indeed make any fix a CVE.

If you really want to build a trust around the CVE process then make it
more transparent. I would start by adding reason why something has been
marked CVE. You are saying there is peer review process going on then
why not add Reviewed-by: to make it explicit and visible.

Thanks!

-- 
Michal Hocko
SUSE Labs

Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

[Greg Kroah-Hartman]

On Thu, May 23, 2024 at 10:26:56AM +0200, Michal Hocko wrote:
> On Wed 22-05-24 05:57:38, Greg KH wrote:
> [...]
> > > I completely do get why you do not care about that downstream
> > > engineering cost but generating bogus CVEs on top of a huge pile of
> > > dubious ones is less than useful, don't you think?
> > 
> > How is this a "bogus" CVE on their own?
> 
> I suspect you haven't looked at those commits. This is a boot time
> suboptimal HW configuration. There is no way any user can exploit that I
> can see. Not to mention it cases system boot hangs!

Yes, you are correct, the original should not have had a CVE assigned to
it, that was wrong, and I have rejected it now.  Same for the revert, it
too is now rejected.  Thanks for the review, it is much appreciated.

Also, the reason the original had a cve assigned to it was a fault on my
side, that shouldn't have happened, and I've re-reviewed to make sure
that I didn't mark anything else that way as well (so far I have not
found anything, the 'revert' caused problems in our tools, not to blame
them, but me, the author of that tool...)

> [...]
> > > Seriously, we can disagree whether something is a security threat that
> > > is worth marking by a CVE. But making the CVE generation process mostly
> > > unattended script driven process without any _serious_ review in place
> > > is burning a lot of man power that could be used in a much more
> > > productive way. This is not the way how to convince people to use stable
> > > kernels.
> > 
> > To think that any of this is an "unattended script without any _serious_
> > review" is slandering all of the people who put in their free time to do
> > this work for you and the community.  This is ANYTHING BUT an unattended
> > script.
> 
> This is a perception one can easily draw by watching the stream of
> incoming flow. Sorry if that is not the case but there is about zero
> transparency about the process except for Documentation/process/cve.rst
> and let me quote
> "
> As part of the normal stable release process, kernel changes that are
> potentially security issues are identified by the developers responsible
> for CVE number assignments and have CVE numbers automatically assigned
> to them.
> "
> 
> There is no mention about criteria, review process. Who is involve in
> the assignment and who is doing the review. The vulns.git tree doesn't
> contain Sign-off-by of those involved parties except for the submitter.

The "criteria" is that as described by cve.org, we can't do anything
about that.  The process, yes, we can be more open about that but as it
has been evolving over time, it's hard to describe a moving target at
times.  I know Lee is writing up an article about how this all works,
and I'm going to be giving talks at conferences in a few months as well.
And people also just ask us directly, which you can :)

Because of asking, many others are starting to help out, you can too,
just submit patches against the cve/review/proposed/ directory with a
list of commits that you feel should have CVEs assigned for, or annotate
why you feel specific ones we have reviewed should NOT have a CVE
assigned, and our tools can handle them quite well as part of the
assignment process (see scripts/cve_review for a tool that some of us
use to create these files, that's not required, as not all of us use it,
but the output format is the key, and that's a simple list of commit
ids, personally I generate that from mboxes.)

> > Also, this is work ostensibly that you are also already doing for your
> > day-job, right?
> 
> We, like stable trees, rely on Fixes tag and those (including other
> commits that might be this tag) are reviewed by domain experts.

Great, so you already have reviewed all of these, so it should be a
simple match up on your end.

> I have raised a concern based on observed CVE flow that the current
> process is automated without a proper review as I can see very dubious
> CVEs being assigned (splats resembling oops/warnings coming from lockdep,
> data_race fixes as they resemble race condition fixes, READ_ONCE fixes
> which do not fix anything discussed in other thread and others).

It's a learning process for all of us involved, and I thank you for your
reviews to help make this better.

> I have tried to dispute quite some but quickly learned that many of them
> have been dismissed because "no usecases are assumed". It is a very
> broad category that could indeed make any fix a CVE.

We can't assume usecases, sorry, you know that.  And yes, it does make
it such that many broad categories can get a CVE, which is just part of
the "business" when working at this low level in the stack.

> If you really want to build a trust around the CVE process then make it
> more transparent. I would start by adding reason why something has been
> marked CVE. You are saying there is peer review process going on then
> why not add Reviewed-by: to make it explicit and visible.

We have that, see the git log of the cve/review/ directory for the files
and where most all of the cves come from.  Some come directly from
requests by others to us, and a few other places (i.e. security
reports), so we of course can't document the source of everything for
obvious reasons.  But always feel free to ask if you think something
looks "odd" and we'll do the best that we can to answer it.

thanks,

greg k-h

Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

[Michal Hocko]

On Thu 23-05-24 15:49:59, Greg KH wrote:
> On Thu, May 23, 2024 at 10:26:56AM +0200, Michal Hocko wrote:
> > On Wed 22-05-24 05:57:38, Greg KH wrote:
> > [...]
> > > > I completely do get why you do not care about that downstream
> > > > engineering cost but generating bogus CVEs on top of a huge pile of
> > > > dubious ones is less than useful, don't you think?
> > > 
> > > How is this a "bogus" CVE on their own?
> > 
> > I suspect you haven't looked at those commits. This is a boot time
> > suboptimal HW configuration. There is no way any user can exploit that I
> > can see. Not to mention it cases system boot hangs!
> 
> Yes, you are correct, the original should not have had a CVE assigned to
> it, that was wrong, and I have rejected it now.  Same for the revert, it
> too is now rejected.  Thanks for the review, it is much appreciated.

Thanks for considering the feedback!

> Also, the reason the original had a cve assigned to it was a fault on my
> side, that shouldn't have happened, and I've re-reviewed to make sure
> that I didn't mark anything else that way as well (so far I have not
> found anything, the 'revert' caused problems in our tools, not to blame
> them, but me, the author of that tool...)

Good to hear this has an explanation.

[...]
> Because of asking, many others are starting to help out, you can too,
> just submit patches against the cve/review/proposed/ directory with a
> list of commits that you feel should have CVEs assigned for, or annotate
> why you feel specific ones we have reviewed should NOT have a CVE
> assigned, and our tools can handle them quite well as part of the
> assignment process (see scripts/cve_review for a tool that some of us
> use to create these files, that's not required, as not all of us use it,
> but the output format is the key, and that's a simple list of commit
> ids, personally I generate that from mboxes.)

Do I get it right that proposals shouldn't be sent via email to
cve@kernel.org as suggested by the in tree documentation? I do not mind
the specific workflow but until now I have followed Documentation/process/cve.rst
as authoritative source of the process. It would be really great if that
matched the workflow.

> > > Also, this is work ostensibly that you are also already doing for your
> > > day-job, right?
> > 
> > We, like stable trees, rely on Fixes tag and those (including other
> > commits that might be this tag) are reviewed by domain experts.
> 
> Great, so you already have reviewed all of these, so it should be a
> simple match up on your end.

Not at all. Every incoming CVE has to go through CVSS assessment at
least. This on its own is a very non-trivial and time consuming task
that obviously scales with the number of CVEs. There is more going
on besides that. 

[...]

> > If you really want to build a trust around the CVE process then make it
> > more transparent. I would start by adding reason why something has been
> > marked CVE. You are saying there is peer review process going on then
> > why not add Reviewed-by: to make it explicit and visible.
> 
> We have that, see the git log of the cve/review/ directory for the files
> and where most all of the cves come from.  Some come directly from
> requests by others to us, and a few other places (i.e. security
> reports), so we of course can't document the source of everything for
> obvious reasons.

Thanks for pointing me there but I do not think this is what I've had in
mind. I do understand that there is some pattern matching happening to
select most of the candidates, but as you've said this is then reviewed
and during that review you likely need to read through that changelog
and build some sort of statement why this is considered a security
threat.

I believe exactly _this_ would be a much more valuable information in
the CVE announcement than a copy&past of the changelog which on its own
can be trially referenced by a link. Also if there is a peer review
happening then Reviewed-by would be really nice. Not to mention
Reported-by if this was externally reported (the report could be a part
of the announcement as well).

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

[Greg Kroah-Hartman]

On Fri, May 24, 2024 at 12:10:56PM +0200, Michal Hocko wrote:
> On Thu 23-05-24 15:49:59, Greg KH wrote:
> > On Thu, May 23, 2024 at 10:26:56AM +0200, Michal Hocko wrote:
> > > On Wed 22-05-24 05:57:38, Greg KH wrote:
> [...]
> > Because of asking, many others are starting to help out, you can too,
> > just submit patches against the cve/review/proposed/ directory with a
> > list of commits that you feel should have CVEs assigned for, or annotate
> > why you feel specific ones we have reviewed should NOT have a CVE
> > assigned, and our tools can handle them quite well as part of the
> > assignment process (see scripts/cve_review for a tool that some of us
> > use to create these files, that's not required, as not all of us use it,
> > but the output format is the key, and that's a simple list of commit
> > ids, personally I generate that from mboxes.)
> 
> Do I get it right that proposals shouldn't be sent via email to
> cve@kernel.org as suggested by the in tree documentation?

The documentation should say that you _SHOULD_ send proposals to
cve@kernel.org, did we get it wrong somehow?

> I do not mind
> the specific workflow but until now I have followed Documentation/process/cve.rst
> as authoritative source of the process. It would be really great if that
> matched the workflow.

I'm confused as to what in that document is incorrect, care to point it
out?

> > > If you really want to build a trust around the CVE process then make it
> > > more transparent. I would start by adding reason why something has been
> > > marked CVE. You are saying there is peer review process going on then
> > > why not add Reviewed-by: to make it explicit and visible.
> > 
> > We have that, see the git log of the cve/review/ directory for the files
> > and where most all of the cves come from.  Some come directly from
> > requests by others to us, and a few other places (i.e. security
> > reports), so we of course can't document the source of everything for
> > obvious reasons.
> 
> Thanks for pointing me there but I do not think this is what I've had in
> mind. I do understand that there is some pattern matching happening to
> select most of the candidates, but as you've said this is then reviewed
> and during that review you likely need to read through that changelog
> and build some sort of statement why this is considered a security
> threat.

Right now for the 3 people doing all of the reviews, 1 is using pattern
matching of a sort (see the cve_review tool for the big regex and
workflow used there), one is reading each patch/header in mbox format,
and one is using some other tool.  Then for the ones that we disagree on
(i.e. not a score of 2 out of 3), we comment as to why we feel they
should, or should not, be accepted.  As this process is evolving, we
haven't really documented it, except here and in talks with others as we
travel.  We're working on making that more public over time.

Note, I do not know of any other CNA that does this in public as much as
we are already, we are pushing the boundry of what CNAs are doing pretty
hard here by putting almost all of our reviews in public _before_ a CVE
is assigned.  That's not normal, and hopefully we don't get told to stop
that (sshhh, don't tell anyone...)

> I believe exactly _this_ would be a much more valuable information in
> the CVE announcement than a copy&past of the changelog which on its own
> can be trially referenced by a link. Also if there is a peer review
> happening then Reviewed-by would be really nice. Not to mention
> Reported-by if this was externally reported (the report could be a part
> of the announcement as well).

We can't do a "Reported-by:" for CVEs as we aren't allowed to add
personal information like that to the records as per cve.org's rules.
And people want to word-smith the text all the time already, so we just
default to using the changelog text as that's the most "neutral" and
public information out there (i.e. we don't have to worry about any sort
of data-retention or classification laws as the information is already
public in kernel changelog text.)

Same for "reviewed-by:" we don't want to do that, as again, no personal
information can be added to cve records, but as we are putting our
reviews in a public repo, you can work backwards if you are curious from
there.

And again, others are sending us reviews as well, you can see them in
the repo already.  Hopefully more will do the same as time goes by, and
we have a steady stream of developers at various companies sending us
requests as well just through email and we handle them that way.

We do have one CVE right now that does want the text changed, and we are
working on making that possible (by accepting patches to the text) as
the changelog text was incorrect and called out an incorrect device as
being a problem when it was not.  But that's an outlier and we will
handle that on a case-by-case basis.

thanks,

greg k-h

Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

[Michal Hocko]

On Fri 24-05-24 13:47:00, Greg KH wrote:
> On Fri, May 24, 2024 at 12:10:56PM +0200, Michal Hocko wrote:
> > On Thu 23-05-24 15:49:59, Greg KH wrote:
> > > On Thu, May 23, 2024 at 10:26:56AM +0200, Michal Hocko wrote:
> > > > On Wed 22-05-24 05:57:38, Greg KH wrote:
> > [...]
> > > Because of asking, many others are starting to help out, you can too,
> > > just submit patches against the cve/review/proposed/ directory with a
> > > list of commits that you feel should have CVEs assigned for, or annotate
> > > why you feel specific ones we have reviewed should NOT have a CVE
> > > assigned, and our tools can handle them quite well as part of the
> > > assignment process (see scripts/cve_review for a tool that some of us
> > > use to create these files, that's not required, as not all of us use it,
> > > but the output format is the key, and that's a simple list of commit
> > > ids, personally I generate that from mboxes.)
> > 
> > Do I get it right that proposals shouldn't be sent via email to
> > cve@kernel.org as suggested by the in tree documentation?
> 
> The documentation should say that you _SHOULD_ send proposals to
> cve@kernel.org, did we get it wrong somehow?
> 
> > I do not mind
> > the specific workflow but until now I have followed Documentation/process/cve.rst
> > as authoritative source of the process. It would be really great if that
> > matched the workflow.
> 
> I'm confused as to what in that document is incorrect, care to point it
> out?

Maybe I've just misunderstood the part about sending patches against
cve/review/proposed/. I was thinking about sending pull requests against
vulns.git.
 
> > > > If you really want to build a trust around the CVE process then make it
> > > > more transparent. I would start by adding reason why something has been
> > > > marked CVE. You are saying there is peer review process going on then
> > > > why not add Reviewed-by: to make it explicit and visible.
> > > 
> > > We have that, see the git log of the cve/review/ directory for the files
> > > and where most all of the cves come from.  Some come directly from
> > > requests by others to us, and a few other places (i.e. security
> > > reports), so we of course can't document the source of everything for
> > > obvious reasons.
> > 
> > Thanks for pointing me there but I do not think this is what I've had in
> > mind. I do understand that there is some pattern matching happening to
> > select most of the candidates, but as you've said this is then reviewed
> > and during that review you likely need to read through that changelog
> > and build some sort of statement why this is considered a security
> > threat.
> 
> Right now for the 3 people doing all of the reviews, 1 is using pattern
> matching of a sort (see the cve_review tool for the big regex and
> workflow used there), one is reading each patch/header in mbox format,
> and one is using some other tool.  Then for the ones that we disagree on
> (i.e. not a score of 2 out of 3), we comment as to why we feel they
> should, or should not, be accepted.

Thanks for the clarification.

> As this process is evolving, we
> haven't really documented it, except here and in talks with others as we
> travel.  We're working on making that more public over time.
> 
> Note, I do not know of any other CNA that does this in public as much as
> we are already, we are pushing the boundry of what CNAs are doing pretty
> hard here by putting almost all of our reviews in public _before_ a CVE
> is assigned.  That's not normal, and hopefully we don't get told to stop
> that (sshhh, don't tell anyone...)

I really like and appreciate this part! That is a huge improvement
comparing to the previous process.

> > I believe exactly _this_ would be a much more valuable information in
> > the CVE announcement than a copy&past of the changelog which on its own
> > can be trially referenced by a link. Also if there is a peer review
> > happening then Reviewed-by would be really nice. Not to mention
> > Reported-by if this was externally reported (the report could be a part
> > of the announcement as well).
> 
> We can't do a "Reported-by:" for CVEs as we aren't allowed to add
> personal information like that to the records as per cve.org's rules.

OK, understood. Although I do remember CVEs crediting parties
discovering/reporting the issue.

> And people want to word-smith the text all the time already, so we just
> default to using the changelog text as that's the most "neutral" and
> public information out there (i.e. we don't have to worry about any sort
> of data-retention or classification laws as the information is already
> public in kernel changelog text.)

This part I do not understand. What is wrong about a reasoning why
something has been considered a CVE? E.g. something like 
CVE assigned because a potential WARN_ON is fixed and that could panic
with panic_on_warn. Fixed by <URL_TO_LINUS_TREE>

or
CVE assigned because UAF is fixed and those can be generally used to
construct more complex attacks. Fixed by <URL_TO_LINUS_TREE>

etc.
-- 
Michal Hocko
SUSE Labs

Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

[Greg Kroah-Hartman]

On Fri, May 24, 2024 at 04:02:18PM +0200, Michal Hocko wrote:
> On Fri 24-05-24 13:47:00, Greg KH wrote:
> > On Fri, May 24, 2024 at 12:10:56PM +0200, Michal Hocko wrote:
> > > On Thu 23-05-24 15:49:59, Greg KH wrote:
> > > > On Thu, May 23, 2024 at 10:26:56AM +0200, Michal Hocko wrote:
> > > > > On Wed 22-05-24 05:57:38, Greg KH wrote:
> > > [...]
> > > > Because of asking, many others are starting to help out, you can too,
> > > > just submit patches against the cve/review/proposed/ directory with a
> > > > list of commits that you feel should have CVEs assigned for, or annotate
> > > > why you feel specific ones we have reviewed should NOT have a CVE
> > > > assigned, and our tools can handle them quite well as part of the
> > > > assignment process (see scripts/cve_review for a tool that some of us
> > > > use to create these files, that's not required, as not all of us use it,
> > > > but the output format is the key, and that's a simple list of commit
> > > > ids, personally I generate that from mboxes.)
> > > 
> > > Do I get it right that proposals shouldn't be sent via email to
> > > cve@kernel.org as suggested by the in tree documentation?
> > 
> > The documentation should say that you _SHOULD_ send proposals to
> > cve@kernel.org, did we get it wrong somehow?
> > 
> > > I do not mind
> > > the specific workflow but until now I have followed Documentation/process/cve.rst
> > > as authoritative source of the process. It would be really great if that
> > > matched the workflow.
> > 
> > I'm confused as to what in that document is incorrect, care to point it
> > out?
> 
> Maybe I've just misunderstood the part about sending patches against
> cve/review/proposed/. I was thinking about sending pull requests against
> vulns.git.

Yes, you can do that too, send it to same email address.  That's not
really documented, just ask us instead!  :)

> > And people want to word-smith the text all the time already, so we just
> > default to using the changelog text as that's the most "neutral" and
> > public information out there (i.e. we don't have to worry about any sort
> > of data-retention or classification laws as the information is already
> > public in kernel changelog text.)
> 
> This part I do not understand. What is wrong about a reasoning why
> something has been considered a CVE? E.g. something like 
> CVE assigned because a potential WARN_ON is fixed and that could panic
> with panic_on_warn. Fixed by <URL_TO_LINUS_TREE>
> 
> or
> CVE assigned because UAF is fixed and those can be generally used to
> construct more complex attacks. Fixed by <URL_TO_LINUS_TREE>
> 
> etc.

Doing the work to classify all of these in this manner isn't going to
happen by us, sorry, as it is not required by the CVE process, and
frankly, we are doing a load of work already here.  We are going to rely
on the text that is in the changelog.  Maybe over time you can work with
the kernel developers to write better changelogs to describe what you
are looking for?

We will rely on external parties to "classify" the CVEs if they wish to
as there is already a whole ecosystem that attempts to do this already,
with various success.  In the end, it's up to each integrator of Linux
to classify them themselves as everyone's use case is different
(remember, cow milking machines, super-mega-yaht-stabilizers, washing
machines, servers, watches, air-traffic-control systems, etc...)

thanks,

greg k-h

Re: CVE-2024-35906: drm/amd/display: Send DTBCLK disable message on first commit

[Michal Hocko]

On Fri 24-05-24 17:22:24, Greg KH wrote:
[...]
> > > And people want to word-smith the text all the time already, so we just
> > > default to using the changelog text as that's the most "neutral" and
> > > public information out there (i.e. we don't have to worry about any sort
> > > of data-retention or classification laws as the information is already
> > > public in kernel changelog text.)
> > 
> > This part I do not understand. What is wrong about a reasoning why
> > something has been considered a CVE? E.g. something like 
> > CVE assigned because a potential WARN_ON is fixed and that could panic
> > with panic_on_warn. Fixed by <URL_TO_LINUS_TREE>
> > 
> > or
> > CVE assigned because UAF is fixed and those can be generally used to
> > construct more complex attacks. Fixed by <URL_TO_LINUS_TREE>
> > 
> > etc.
> 
> Doing the work to classify all of these in this manner isn't going to
> happen by us, sorry, as it is not required by the CVE process, and
> frankly, we are doing a load of work already here.

I would really like the understand this position. You are doing the
classification already, right? What does prevent you from making that a
part of the process? Why wouldn't you like to help CVE consumers to
understand that process better?
-- 
Michal Hocko
SUSE Labs