Re: CVE-2023-52734: net: sched: sch: Bounds check priority

Re: CVE-2023-52734: net: sched: sch: Bounds check priority

[Michal Hocko]

Is this really soemthing that should be getting a CVE assigned?
First the fix is incomplete - 9cec2aaffe96 ("net: sched: sch: Fix off by one in htb_activate_prios()")
Second is this even real problem? https://lore.kernel.org/all/Y9V3mBmLUcrEdrTV@pop-os.localdomain/
suggests it is not.

And third, WARN_ONs are considered a real deal by CVE team because
somebody might be running with panic_on_warn. This patch adds one!

On Tue 21-05-24 17:23:11, Greg KH wrote:
> Description
> ===========
> 
> In the Linux kernel, the following vulnerability has been resolved:
> 
> net: sched: sch: Bounds check priority
> 
> Nothing was explicitly bounds checking the priority index used to access
> clpriop[]. WARN and bail out early if it's pathological. Seen with GCC 13:
> 
> ../net/sched/sch_htb.c: In function 'htb_activate_prios':
> ../net/sched/sch_htb.c:437:44: warning: array subscript [0, 31] is outside array bounds of 'struct htb_prio[8]' [-Warray-bounds=]
>   437 |                         if (p->inner.clprio[prio].feed.rb_node)
>       |                             ~~~~~~~~~~~~~~~^~~~~~
> ../net/sched/sch_htb.c:131:41: note: while referencing 'clprio'
>   131 |                         struct htb_prio clprio[TC_HTB_NUMPRIO];
>       |                                         ^~~~~~
> 
> The Linux kernel CVE team has assigned CVE-2023-52734 to this issue.
> 
> 
> Affected and fixed versions
> ===========================
> 
> 	Fixed in 5.4.232 with commit fbe71c5dacaa
> 	Fixed in 5.10.169 with commit 90fcf55d83b2
> 	Fixed in 5.15.95 with commit 99875ea9b5b4
> 	Fixed in 6.1.13 with commit f6415c9c9a0b
> 	Fixed in 6.2 with commit de5ca4c3852f
> 
> 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-2023-52734
> 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:
> 	net/sched/sch_htb.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/fbe71c5dacaa5a9960323215f118958174c81aa0
> 	https://git.kernel.org/stable/c/90fcf55d83b20da1091f926a291af05fb74f61c6
> 	https://git.kernel.org/stable/c/99875ea9b5b47995bfb3c684d21eb17feb4b7e6a
> 	https://git.kernel.org/stable/c/f6415c9c9a0b3881543d38528a58b54af4351522
> 	https://git.kernel.org/stable/c/de5ca4c3852f896cacac2bf259597aab5e17d9e3

-- 
Michal Hocko
SUSE Labs

Re: CVE-2023-52734: net: sched: sch: Bounds check priority

[Greg Kroah-Hartman]

On Tue, May 28, 2024 at 09:53:12AM +0200, Michal Hocko wrote:
> Is this really soemthing that should be getting a CVE assigned?
> First the fix is incomplete - 9cec2aaffe96 ("net: sched: sch: Fix off by one in htb_activate_prios()")

Incomplete fixes are still part of a fix :)

> Second is this even real problem? https://lore.kernel.org/all/Y9V3mBmLUcrEdrTV@pop-os.localdomain/
> suggests it is not.

Ah, good catch, I didn't see that.  I'll go revoke this as it's not
doing anything.

> And third, WARN_ONs are considered a real deal by CVE team because
> somebody might be running with panic_on_warn. This patch adds one!

Yes, but if you can't hit that by anything from userspace, it's not an
issue and just dead code.  We'll have to wait for a future syzbot report
to prove that wrong :)

thanks for the review!

greg k-h

Re: CVE-2023-52734: net: sched: sch: Bounds check priority

[Michal Hocko]

On Tue 28-05-24 21:06:39, Greg KH wrote:
> On Tue, May 28, 2024 at 09:53:12AM +0200, Michal Hocko wrote:
> > Is this really soemthing that should be getting a CVE assigned?
> > First the fix is incomplete - 9cec2aaffe96 ("net: sched: sch: Fix off by one in htb_activate_prios()")
> 
> Incomplete fixes are still part of a fix :)

Sigh

> > Second is this even real problem? https://lore.kernel.org/all/Y9V3mBmLUcrEdrTV@pop-os.localdomain/
> > suggests it is not.
> 
> Ah, good catch, I didn't see that.  I'll go revoke this as it's not
> doing anything.

Thanks!

I wish the CVE review process would catch something like that before
issuing a CVE for it.

> > And third, WARN_ONs are considered a real deal by CVE team because
> > somebody might be running with panic_on_warn. This patch adds one!
> 
> Yes, but if you can't hit that by anything from userspace, it's not an
> issue and just dead code.  We'll have to wait for a future syzbot report
> to prove that wrong :)

I am not judging the patch itself. It is maintainers who should decide
whether this is something they want to accept.

I am questioning the decision to make it a CVE. Because if that was a
real deal then WARN_ON is something kernel CNA is considering a CVE worth
problem! So a CVE has been filed with a fix that is CVE itself.
Seriously how could this pass through the CVE review process?
-- 
Michal Hocko
SUSE Labs

Re: CVE-2023-52734: net: sched: sch: Bounds check priority

[Greg Kroah-Hartman]

On Wed, May 29, 2024 at 09:30:08AM +0200, Michal Hocko wrote:
> On Tue 28-05-24 21:06:39, Greg KH wrote:
> > On Tue, May 28, 2024 at 09:53:12AM +0200, Michal Hocko wrote:
> > > Is this really soemthing that should be getting a CVE assigned?
> > > First the fix is incomplete - 9cec2aaffe96 ("net: sched: sch: Fix off by one in htb_activate_prios()")
> > 
> > Incomplete fixes are still part of a fix :)
> 
> Sigh
> 
> > > Second is this even real problem? https://lore.kernel.org/all/Y9V3mBmLUcrEdrTV@pop-os.localdomain/
> > > suggests it is not.
> > 
> > Ah, good catch, I didn't see that.  I'll go revoke this as it's not
> > doing anything.
> 
> Thanks!
> 
> I wish the CVE review process would catch something like that before
> issuing a CVE for it.

I too want a pony :)

> > > And third, WARN_ONs are considered a real deal by CVE team because
> > > somebody might be running with panic_on_warn. This patch adds one!
> > 
> > Yes, but if you can't hit that by anything from userspace, it's not an
> > issue and just dead code.  We'll have to wait for a future syzbot report
> > to prove that wrong :)
> 
> I am not judging the patch itself. It is maintainers who should decide
> whether this is something they want to accept.
> 
> I am questioning the decision to make it a CVE. Because if that was a
> real deal then WARN_ON is something kernel CNA is considering a CVE worth
> problem! So a CVE has been filed with a fix that is CVE itself.
> Seriously how could this pass through the CVE review process?

"How" is "this was part of the entries in the GSD records that MITRE
asked us to back-fill as CVE entries".  Those entries already went
through two different rounds of review last year for the GSD record, and
I did another one as well now before the CVE creation happened.  It was
in a batch where I reviewed 124 entries at once, and if I only got one
wrong, hey, that's a very good % overall, don't you think?  Especially
as it has been a publicily listed "vulnerability fix" for well over a
year now in the GSD system, and no one objected to it there.

Yes, I will get things wrong on the GSD backfill, and I am re-reviewing
them all, which is honestly, something that MITRE did NOT ask us to do,
but I am doing just because of minor things like this where perhaps the
entry should not have been made in the past.  And as part of that
review, yes, I have found some other entries that didn't deserve a CVE,
and so didn't create them.  If you would like me to just do a simple
"import them all without an additional review", I will be glad to do so
as it would be much easier and save me loads of time.

I welcome others to help out with this work, including yourself, if you
so desire.  That would help out a lot.

thanks,

greg k-h

Re: CVE-2023-52734: net: sched: sch: Bounds check priority

[Michal Hocko]

On Wed 29-05-24 11:51:10, Greg KH wrote:
> On Wed, May 29, 2024 at 09:30:08AM +0200, Michal Hocko wrote:
[...]
> > I am questioning the decision to make it a CVE. Because if that was a
> > real deal then WARN_ON is something kernel CNA is considering a CVE worth
> > problem! So a CVE has been filed with a fix that is CVE itself.
> > Seriously how could this pass through the CVE review process?
> 
> "How" is "this was part of the entries in the GSD records that MITRE
> asked us to back-fill as CVE entries".  Those entries already went
> through two different rounds of review last year for the GSD record, and
> I did another one as well now before the CVE creation happened.  

I am sorry but I have no idea how that is supposed to justify assigning
a CVE to a non-issue with a fix that is clearly considered a CVE on its
own. An overlook, sure. That is understandable but the above doesn't
make any sense to me.

> It was in a batch where I reviewed 124 entries at once,

OK, this makes much more sense and I do not mean to blame you for
overlooking a particular things. But ...

> and if I only got one
> wrong, hey, that's a very good % overall, don't you think?  Especially
> as it has been a publicily listed "vulnerability fix" for well over a
> year now in the GSD system, and no one objected to it there.

... it is unavoidable to overlook completely bogus or even harmfull CVE
fixes if they are generated in the current volumes. It is much worse
that it is easier to overlook those which really are important.
Especially during CVSS assessment. This simply cannot scale!

[...]

> I welcome others to help out with this work, including yourself, if you
> so desire.  That would help out a lot.

I am sorry but I fundamentally disagree with the way how CVEs are
processed _now_ and therefore I will not put my name under that. I am
still hopeful that this is just the new process finding its own way and
it will settle on something much more reasonable and _useful_.

-- 
Michal Hocko
SUSE Labs