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

[Pasha Tatashin]

On Tue, Mar 31, 2026 at 5:40 AM Pratyush Yadav <pratyush@xxxxxxxxxx> wrote:
>
> 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.

It is one line change, I am going to add as well. Thanks.

>
> --
> Regards,
> Pratyush Yadav