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
