Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
New issues:
- [Critical] Use-After-Free window during active dictionary reallocation for inverse translation maps.
- [High] Unicode characters >= 512 are incorrectly rejected in conv_uni_to_pc when CONFIG_FB_GLYPH_21BIT is disabled.
- [High] dict->count is misused as the maximum font glyph index, is uninitialized for default maps, and causes functional breakage.
- [Medium] Empty glyph slots are incorrectly copied due to C integer promotion in con_unshare_unimap when using 16-bit values.
- [Medium] Incorrect allocation size calculation wastes memory for user-controlled allocations.
- [Low] Label at the end of a compound statement without a trailing null statement causes a build failure.
Pre-existing issues:
- [Critical] Concurrent read UAF in con_unshare_unimap error path.
--
commit 58f6de25804fcafcee92f219003205102885c384
Author: Alan Mackenzie <[email protected]>
vt: 32b glyph: 1. Make consolemap.c handle Unicode planes outside BMP
This commit adds a fourth layer to the sparse map in consolemap.c to handle
Unicode planes outside the Basic Multilingual Plane (BMP). It increases the
supported glyph number limit up to 2^21 and shifts user-to-kernel data
conversions to vt_ioctl.c.
> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> index 3fa89a2dbebab..2f695d97f0ebd 100644
> --- a/drivers/tty/vt/consolemap.c
> +++ b/drivers/tty/vt/consolemap.c
[ ... ]
> @@ -235,23 +248,22 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *dict,
> {
[ ... ]
> - if (!inv) {
> - inv = dict->inverse_translations[m] = kmalloc(MAX_GLYPH,
> - GFP_KERNEL);
> - if (!inv)
> - return;
> - }
> - memset(inv, 0, MAX_GLYPH);
> + kfree(inv);
> + inv = dict->inverse_translations[m] =
> + kmalloc(dict->count * (sizeof(*t)), GFP_KERNEL);
[Severity: Critical]
Does this create a Use-After-Free window? Since GFP_KERNEL allocations can
sleep, freeing the active dictionary array and leaving the pointer populated
before the reallocation completes might expose freed memory to concurrent
lockless readers (like accessibility driver keyboard handlers) calling
inverse_translate().
[Severity: Medium]
Also, does this calculation allocate more memory than intended? inv is an
unsigned char pointer, but the allocation uses sizeof(*t) where t is a
u1632 pointer. This allocates 2-4x the needed size for 8-bit inverse maps
based on a user-controlled element count.
> + if (!inv)
> + return;
> + memset(inv, 0, dict->count * (sizeof(*t)));
[ ... ]
> @@ -252,39 +265,48 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *dict,
> static void set_inverse_trans_unicode(struct uni_pagedict *dict)
> {
[ ... ]
> - if (!inv) {
> - inv = dict->inverse_trans_unicode = kmalloc_array(MAX_GLYPH,
> - sizeof(*inv), GFP_KERNEL);
> - if (!inv)
> - return;
> - }
> - memset(inv, 0, MAX_GLYPH * sizeof(*inv));
> + kfree(inv);
> + inv = dict->inverse_trans_unicode =
> + kmalloc_array(dict->count, sizeof(*inv), GFP_KERNEL);
[Severity: Critical]
Similar to the 8-bit inverse translation map above, could this expose a freed
pointer to concurrent readers while waiting on the GFP_KERNEL allocation?
> + if (!inv)
> + return;
> + memset(inv, 0, dict->count * sizeof(*inv));
[ ... ]
> @@ -297,18 +319,19 @@ unsigned short *set_translate(enum translation_map m, struct vc_data *vc)
> * was active.
> * Still, it is now possible to a certain extent to cut and paste non-ASCII.
> */
> -u16 inverse_translate(const struct vc_data *conp, u16 glyph, bool use_unicode)
> +u1632 inverse_translate(const struct vc_data *conp, u1632 glyph,
> + bool use_unicode)
> {
> struct uni_pagedict *p;
> enum translation_map m;
>
> - if (glyph >= MAX_GLYPH)
> - return 0;
> -
> p = *conp->uni_pagedict_loc;
> if (!p)
> return glyph;
>
> + if (glyph >= p->count)
> + return 0;
[Severity: High]
Is p->count the correct upper boundary for a font index here? dict->count
is initialized to the number of user-provided mapping pairs, which might be
much smaller than the highest glyph index in the font. This seems like it
will falsely reject valid glyphs.
Furthermore, looking at con_set_default_unimap() (not modified here but
impacted), dict->count is never initialized to 256 for default maps, leaving
it at 0. Won't this break reverse translations for default fonts entirely?
> +
> if (use_unicode) {
> if (!p->inverse_trans_unicode)
> return glyph;
[ ... ]
> @@ -587,63 +639,70 @@ static struct uni_pagedict *con_unshare_unimap(struct vc_data *vc,
[ ... ]
> + for (g = 0; g < UNI_ROW_GLYPHS; g++, uni++) {
> + if (row[g] == ~0)
> + continue;
[Severity: Medium]
Will this optimization check fail when CONFIG_FB_GLYPH_21BIT is disabled?
In that configuration, u1632 is a 16-bit unsigned integer. Due to C integer
promotion, 0xFFFF is zero-extended to 65535, which does not equal the
signed int -1.
It looks like this will result in all uninitialized empty slots being copied
via con_insert_unipair(), wasting memory and corrupting dict->sum by
adding 0xFFFF for each empty slot.
> + /*
> + * Found one, copy entry for unicode
> + * uni with fontpos value row[g].
> + */
> + ret = con_insert_unipair(new, uni,
> + row[g]);
> + if (ret) {
> + old->refcount++;
> + *vc->uni_pagedict_loc = old;
> + con_release_unimap(new);
> + kfree(new);
[Severity: Critical]
This is a pre-existing issue, but does this error path leave a dangling
pointer? The new dictionary was already published to readers globally via
*vc->uni_pagedict_loc = new inside con_allocate_new(). Freeing the new
dict here could cause a Use-After-Free for concurrent lockless readers.
> + return ERR_PTR(ret);
> + }
> }
> }
> }
> }
> -
> return new;
> }
>
> -int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
> +int con_set_unimap(struct vc_data *vc, u32 ct, struct unipair8_21 *list)
> {
[ ... ]
> @@ -660,7 +719,9 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
> /*
> * Insert user specified unicode pairs into new table.
> */
> - for (plist = unilist; ct; ct--, plist++) {
> + err = 0;
> + dict->count = ct;
> + for (plist = list; ct; ct--, plist++) {
[Severity: High]
As mentioned above, assigning ct to dict->count seems to mistakenly track
the total number of user map entries instead of the maximum glyph index, which
breaks subsequent boundary checks.
> int err1 = con_insert_unipair(dict, plist->unicode, plist->fontpos);
> if (err1)
> err = err1;
[ ... ]
> @@ -769,54 +830,69 @@ EXPORT_SYMBOL(con_copy_unimap);
> * Read the console unicode data for this console. Called from the ioctl
> * handlers.
> */
> -int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
> - struct unipair __user *list)
> +int con_get_unimap(struct vc_data *vc, u32 ct, u1632 *uct,
> + struct unipair8_21 *list)
> {
[ ... ]
> + for (g = 0; g < UNI_ROW_GLYPHS; g++, row++) {
> + if (*row >= dict->count)
> + continue;
[Severity: High]
Does this drop valid unicode mappings when reading back the unimap? If *row
is a valid glyph index larger than the total number of mapping pairs
(dict->count), it seems it will be skipped entirely.
> - if (ect < ct) {
> - unilist[ect].unicode = UNI(d, r, g);
> - unilist[ect].fontpos = *row;
> + if (ect < ct) {
> +#ifndef CONFIG_FB_GLYPH_21BIT
> + if ((p || *row > 0xffff)) {
> + ret = -EINVAL;
> + goto unlock;
> + }
> +#endif
> + if (list) {
> + list[ect].unicode = UNI(p, d, r, g);
> + list[ect].fontpos = *row;
> + }
> + }
> + ect++;
> }
> - ect++;
> }
> }
> }
> +#ifndef CONFIG_FB_GLYPH_21BIT
> +unlock:
> +#endif
> }
[Severity: Low]
When CONFIG_FB_GLYPH_21BIT is disabled, will this label at the end of the
compound statement cause a build failure? Older or strict C compilers require
a trailing empty statement (like a semicolon) after a label that sits right
before a closing brace.
> -
> - if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
> - return -EFAULT;
> - if (put_user(ect, uct))
> - return -EFAULT;
> - if (ect > ct)
> - return -ENOMEM;
> -
> - return 0;
> + *uct = ect;
[ ... ]
> @@ -849,26 +925,33 @@ int conv_uni_to_8bit(u32 uni)
> int conv_uni_to_pc(struct vc_data *conp, long ucs)
> {
> struct uni_pagedict *dict;
> - u16 **dir, *row, glyph;
> + u1632 ***plane, **dir, *row, glyph;
>
> - /* Only 16-bit codes supported at this time */
> - if (ucs > 0xffff)
> - return -4; /* Not found */
> + if (ucs >= MAX_GLYPH)
> + return -4; /* Not found */
[Severity: High]
Does this improperly restrict valid Unicode code points when
CONFIG_FB_GLYPH_21BIT is disabled? MAX_GLYPH resolves to 512 in that
configuration.
This validation replaces the old ucs > 0xffff boundary check. Valid Unicode
code points (like Cyrillic, Greek, or Arabic) that are numerically greater
than 512 appear to be erroneously rejected here, breaking console text
rendering for non-Latin alphabets.
> else if (ucs < 0x20)
> return -1; /* Not a printable character */
> + else if (ucs == 0xfeff || (ucs >= 0x200b && ucs <= 0x200f))
> + return -2; /* Zero-width space */
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.