[v2] tty/vt: UTF-8 parsing update according to RFC 3629, modern Unicode

From: Roman Žilka <roman.zilka@gmail.com>

vc_translate_unicode() and vc_sanitize_unicode() parse input to the
UTF-8-enabled console, marking invalid byte sequences and producing Unicode
codepoints. The current algorithm follows ancient Unicode and may accept
invalid byte sequences, pass on non-existent codepoints and reject valid
sequences.

The patch restores the functions' compliance with modern Unicode (v15.1 +
+ many previous versions) as well as RFC 3629.
1. Codepoint space is limited to 0x10FFFF.
2. "Noncharacters", such as U+FFFE, U+FFFF, are no longer invalid in
   Unicode and will be accepted. Another option was to complete the set of
   noncharacters (used to be just those two, now there's more) and preserve
   the rejection step. This is indeed what Unicode suggests (v15.1, chap.
   23.7) (not requires), but most codepoints are !iswprint(), so selecting
   just the noncharacters seemed arbitrary and futile (and unnecessary).

On the side:
3. What remained of vc_sanitize_unicode() is in vc_translate_unicode().
4. Corrected vc_translate_unicode() doc (@rescan).

This is not a security patch. I'm not aware of any present security
implications of the old code.

Signed-off-by: Roman Žilka <roman.zilka@gmail.com>
---
v2: A more elaborate commit msg, e-mail formatting corrections.

 drivers/tty/vt/vt.c | 36 +++++++-----------------------------
 1 file changed, 7 insertions(+), 29 deletions(-)


base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9

On Tue, Dec 12, 2023 at 09:26:53PM +0100, Roman Žilka wrote:
> vc_translate_unicode() and vc_sanitize_unicode() parse input to the
> UTF-8-enabled console, marking invalid byte sequences and producing Unicode
> codepoints. The current algorithm follows ancient Unicode and may accept
> invalid byte sequences, pass on non-existent codepoints and reject valid
> sequences.
> 
> The patch restores the functions' compliance with modern Unicode (v15.1 [1]
> + many previous versions) as well as RFC 3629 [2].
> 1. Codepoint space is limited to 0x10FFFF.

Wait, why?  And shouldn't this be an individual patch on it's own?  What
is wrong with the checking we currently have.

> 2. "Noncharacters", such as U+FFFE, U+FFFF, are no longer invalid in
>    Unicode and will be accepted.

Accepted when?

> Another option was to complete the set of
>    noncharacters (used to be just those two, now there's more) and preserve
>    the rejection step. This is indeed what Unicode suggests (v15.1, chap.
>    23.7) (not requires), but most codepoints are !iswprint(), so selecting
>    just the noncharacters seemed arbitrary and futile (and unnecessary).

What is this change going to break with existing systems that were
thinking these were invalid characters?

> On the side:
> 3. Corrected/improved the doc of the two functions (esp. @rescan).

Again, a separate commit.  When you have to list the changes out, that
is a huge hint it needs to be broken up into smaller pieces.

thanks,

greg k-h

On 1/4/24 4:28 PM, Greg KH wrote:
> On Tue, Dec 12, 2023 at 09:26:53PM +0100, Roman Žilka wrote:
>> vc_translate_unicode() and vc_sanitize_unicode() parse input to the
>> UTF-8-enabled console, marking invalid byte sequences and producing Unicode
>> codepoints. The current algorithm follows ancient Unicode and may accept
>> invalid byte sequences, pass on non-existent codepoints and reject valid
>> sequences.
>>
>> The patch restores the functions' compliance with modern Unicode (v15.1 [1]
>> + many previous versions) as well as RFC 3629 [2].
>> 1. Codepoint space is limited to 0x10FFFF.
> 
> Wait, why?  And shouldn't this be an individual patch on it's own?  What
> is wrong with the checking we currently have.

This is the main point of this patch. The codepoint space got shortened in Unicode at some point between v3.0 (1999) and v4.0 (2003). The reason why is expressed by the first sentence in the commit msg. The affected functions validate input coming into the subsystem from the user, which makes it a red flag that they do not do so correctly (i.e., according to a generally accepted standard). As they stand, these functions are a potential source of compatibility and security issues. The may not be a bomb, they may be a time bomb.

Note how very old the old parsing algorithm is. I made a quick grep of the kernel source for tell-tale signs of utf8 parsing to see if there's any other place where the old algorithm is still being used. I found none, and I found these which do the 0x10ffff limiting (I didn't check the "noncharacters" handling):

fs/unicode/mkutf8data.c
fs/unicode/utf8-norm.c
fs/udf/unicode.c
fs/nls/nls_base.c (has many users outside fs/)
drivers/tty/vt/keyboard.c

I didn't check, but I have no doubt that Perl implements Unicode correctly as well.

>> 2. "Noncharacters", such as U+FFFE, U+FFFF, are no longer invalid in
>>    Unicode and will be accepted.
> 
> Accepted when?

Currently, the two affected functions mark these codepoints as invalid by substituting them with the placeholder U+FFFD. After the patch, U+FFFE and U+FFFF are treated as ordinary valid codepoints.

Let me point out that I've never seen a utf8 validator where "noncharacters" were treated in a special way. Of course, there're only so many validator implementations that I have seen. Checking for iswprint()-ability is common, but that's something very different. IMHO, the validator in vt.c is not a place to pay any special regard to "noncharacters".

>> Another option was to complete the set of
>>    noncharacters (used to be just those two, now there's more) and preserve
>>    the rejection step. This is indeed what Unicode suggests (v15.1, chap.
>>    23.7) (not requires), but most codepoints are !iswprint(), so selecting
>>    just the noncharacters seemed arbitrary and futile (and unnecessary).
> 
> What is this change going to break with existing systems that were
> thinking these were invalid characters?

This is mostly answered above. I don't work with the kernel in a developer capacity. I found this parsing error by accident while researching some CONFIG options. I'm not qualified to say that this patch won't break anything and it would take me an abhorrent amount of time to verify that to a reasonable degree. I don't write code for kbd or other console-related userspace tools either. I ran the patched kernel for a while, played around with fonts and various TUI utilities. I found no issues. The red flag, which I talked about earlier, was _the_ reason I submitted my patch.

>> On the side:
>> 3. Corrected/improved the doc of the two functions (esp. @rescan).
> 
> Again, a separate commit.  When you have to list the changes out, that
> is a huge hint it needs to be broken up into smaller pieces.

Ok, patch v4 coming up with this removed and I'll take care of it in a subsequent submission. That'll be one truly trivial commit, though.

-rz