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

[Peilin Ye]

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