linux-kernel - Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const qualifier

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 2 Nov 2020 11:12:03 -0500
From:   Peilin Ye <yepeilin.cs@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Lee Jones <lee.jones@...aro.org>, daniel.vetter@...ll.ch,
        gregkh@...uxfoundation.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 1/1] Fonts: font_acorn_8x8: Replace discarded const
 qualifier

Hi Russell,

On Mon, Nov 02, 2020 at 10:23:43AM +0000, Russell King - ARM Linux admin wrote:
> On Sun, Nov 01, 2020 at 01:11:22PM +0000, Lee Jones wrote:
> > On Sat, 31 Oct 2020, Russell King - ARM Linux admin wrote:
> > 
> > > On Fri, Oct 30, 2020 at 06:18:22PM +0000, Lee Jones wrote:
> > > > Commit 09e5b3fd5672 ("Fonts: Support FONT_EXTRA_WORDS macros for
> > > 
> > > Your commit ID does not exist in mainline kernels, which makes this
> > > confusing. The commit ID you should be using is 6735b4632def.
> > 
> > Ah yes, quite right.  That is the ID from android-3.18 where this
> > issue was first seen and fixed against.  I will fix it up for
> > Mainline.
> > 
> > Does the fix look okay to you though Russell?
> 
> Frankly, I don't know. Looking at the commit itself, it looks safe,
> but it depends what this "extra" data is being used for. From what
> I can see, the commit in question just adds the additional opaque
> data as a member named "extra", and one is left to guess what it's
> use as.

Thank you very much for looking into this. I apologize for the trouble
and confusion it has caused.

The motivation behind this commit, and commit 5af08640795b ("fbcon: Fix
global-out-of-bounds read in fbcon_get_font()") was to fix a decades-old
out-of-bounds access bug in the framebuffer layer.

However the framebuffer layer is doing bounds checking in a very strange
way, by hiding the buffer length before the buffer, then access it using
a negative-indexing macro:

	#define FNTSIZE(fd)	(((int *)(fd))[-2])

Other "extra" (so-called by the framebuffer layer) fields include:

	#define REFCOUNT(fd)	(((int *)(fd))[-1])

	#define FNTCHARCNT(fd)	(((int *)(fd))[-3])
	#define FNTSUM(fd)	(((int *)(fd))[-4])

...representing reference count, character count and checksum,
respectively.

The commit in question (6735b4632def) prepends the buffer length to each
of the built-in font buffers, so other functions in the framebuffer
layer can use FNTSIZE() on them. 5af08640795b uses it to fix that
out-of-bounds bug.

> I'd have thought a small structure with named members would have
> been the minimum given our standards for in-kernel code.

Yes, this is a temporary bug fix, and is far from satisfactory. We are
trying to replace these magic macros using a structure with properly
named members. It is taking more time than I imagined, but one day this
temporary fix will disappear from the kernel, I hope.

> Why was the "const" dropped in the first place? Does this "extra"
> member get written to somewhere?

No, I will try to come up with a solution without these fields being
writable.

> So, sorry, no idea. This looks to me like a very unsatisfactory
> commit, and probably something that got a very poor review.

I hope this helps explain it.

Again, I apologize for all the troubles. I will do more thorough testing
and practice writing a commit message. Thank you!

Sincerely,
Peilin Ye