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