Re: [PATCH v3 01/10] liveupdate: Safely print untrusted strings

[Pratyush Yadav]

Hi Pasha,

On Fri, Mar 27 2026, Pasha Tatashin wrote:

> On Thu, Mar 26, 2026 at 11:33 PM Pasha Tatashin
> <pasha.tatashin@xxxxxxxxxx> wrote:
>>
>> Deserialized strings from KHO data (such as file handler compatible
>> strings and session names) are provided by the previous kernel and
>> might not be null-terminated if the data is corrupted or maliciously
>> crafted.
>>
>> When printing these strings in error messages, use the %.*s format
>> specifier with the maximum buffer size to prevent out-of-bounds reads
>> into adjacent kernel memory.
>>
>> Signed-off-by: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx>
>> ---
>>  kernel/liveupdate/luo_file.c    | 3 ++-
>>  kernel/liveupdate/luo_session.c | 3 ++-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/liveupdate/luo_file.c b/kernel/liveupdate/luo_file.c
>> index 5acee4174bf0..a6d98fc75d25 100644
>> --- a/kernel/liveupdate/luo_file.c
>> +++ b/kernel/liveupdate/luo_file.c
>> @@ -785,7 +785,8 @@ int luo_file_deserialize(struct luo_file_set *file_set,
>>                 }
>>
>>                 if (!handler_found) {
>> -                       pr_warn("No registered handler for compatible '%s'\n",
>> +                       pr_warn("No registered handler for compatible '%.*s'\n",
>> +                               (int)sizeof(file_ser[i].compatible),
>>                                 file_ser[i].compatible);
>>                         return -ENOENT;
>>                 }
>> diff --git a/kernel/liveupdate/luo_session.c b/kernel/liveupdate/luo_session.c
>> index 25ae704d7787..8c76dece679b 100644
>> --- a/kernel/liveupdate/luo_session.c
>> +++ b/kernel/liveupdate/luo_session.c
>> @@ -544,7 +544,8 @@ int luo_session_deserialize(void)
>>
>>                 session = luo_session_alloc(sh->ser[i].name);
>>                 if (IS_ERR(session)) {
>> -                       pr_warn("Failed to allocate session [%s] during deserialization %pe\n",
>> +                       pr_warn("Failed to allocate session [%.*s] during deserialization %pe\n",
>> +                               (int)sizeof(sh->ser[i].name),
>>                                 sh->ser[i].name, session);
>>                         return PTR_ERR(session);
>>                 }
>
> Lol, Sashiko went a little overboard and gave this patch two
> "Critical" findings:
>
> 1. If a registered file handler uses a compatible string equal to or longer than
> the buffer, and the untrusted string matches it without a null terminator,
> strcmp() could read past the bounds of file_ser[i].compatible.
>
> B.S.: The length of the string is ABI, and fh->compatible is a
> NULL-terminated string provided by the current kernel. In the future,
> we can replace strcmp() with strncmp(), but it is not a high-priority
> issue.
>
> 2. By returning PTR_ERR(session) directly without updating the static err
> variable, subsequent calls will see is_deserialized as true and return 0.
>
> This is regarding luo_session_deserialize(), that is the intended
> behavior. We attempt deserialization exactly once, and if it fails,
> some resources stay "leaked" and inaccessible to the user until the
> next reboot. This is the safest approach to avoid data leaks.

I think you misunderstood. Sashiko brings up a very good point. The
problem is not that we don't attempt the deserialization again, the
problem is that this code path doesn't set err.

So this results in is_deserialized == true, but err == 0 even though
deserialization failed. So the next attempt to open /dev/liveupdate will
succeed since

	if (is_deserialized)
		return err;

will return 0. So I think you need to do:

	err = PTR_ERR(session);
	return err;

To make sure this error code gets recorded and the next open of
/dev/liveupdate also fails.

Anyway, this isn't directly related to this patch but it is a real bug
that should be fixed in a separate patch.

-- 
Regards,
Pratyush Yadav