Re: [RFC PATCH v4 3/4] livepatch: Implement replace set for scoped atomic replace

On Tue, Aug 4, 2026 at 3:03=E2=80=AFPM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issu=
e(s) to consider:
> - [Medium] Dynamically allocated `funcs` arrays are leaked on KLP_OBSOLET=
ES parsing failure.
> --
>
> --- Patch [3]: [RFC PATCH v4 3/4] livepatch: Implement replace set for sc=
oped atomic replace ---
> commit d186609c1a3dbe210857d0040f657fa918504faf
> Author: Yafang Shao <[email protected]>
> Subject: livepatch: Implement replace set for scoped atomic replace
>
> This commit introduces a more flexible atomic replace model for livepatch=
es
> using provides and obsoletes fields. A new patch will replace any existin=
g
> patch with a matching provides ID or any ID listed in its obsoletes array=
.
> The klp-build script and init code are updated to parse and validate thes=
e
> new fields.
>
> > diff --git a/scripts/livepatch/init.c b/scripts/livepatch/init.c
> > --- a/scripts/livepatch/init.c
> > +++ b/scripts/livepatch/init.c
> [ ... ]
> > @@ -72,10 +73,73 @@ static int __init livepatch_mod_init(void)
> [ ... ]
> > +#ifdef KLP_OBSOLETES
> > +     /*
> > +      * Parse KLP_OBSOLETES string (format: "0,1,2" or "0" or empty)
> > +      * and convert to unsigned int array for patch->obsoletes
> > +      *
> > +      * Note: KLP_OBSOLETES is always defined and includes at least th=
e
> > +      * provides ID, ensuring the patch replaces old patches with same=
 ID.
> > +      */
> > +     {
> > +             unsigned int *obs_array;
> > +             unsigned int count =3D 1;
> > +             char *obsoletes_str;
> > +             char *token, *str;
> > +             int i =3D 0;
> > +
> > +             for (str =3D (char *)KLP_OBSOLETES; *str; str++) {
> > +                     if (*str =3D=3D ',')
> > +                             count++;
> > +             }
> > +
> > +             obsoletes_str =3D kstrdup(KLP_OBSOLETES, GFP_KERNEL);
> > +             if (!obsoletes_str) {
> > +                     ret =3D -ENOMEM;
> > +                     goto err_free_objs;
> > +             }
> > +
> > +             obs_array =3D kmalloc_array(count, sizeof(unsigned int), =
GFP_KERNEL);
> > +             if (!obs_array) {
> > +                     kfree(obsoletes_str);
> > +                     ret =3D -ENOMEM;
> > +                     goto err_free_objs;
> > +             }
> > +
> > +             str =3D obsoletes_str;
> > +             while ((token =3D strsep(&str, ",")) !=3D NULL) {
> > +                     unsigned int val;
> > +
> > +                     ret =3D kstrtouint(token, 10, &val);
> > +                     if (ret) {
> > +                             kfree(obsoletes_str);
> > +                             kfree(obs_array);
> > +                             goto err_free_objs;
> > +                     }
> > +                     obs_array[i++] =3D val;
> > +             }
>
> [Severity: Medium]
> Does this code leak the funcs arrays on error?
>
> Earlier in livepatch_mod_init(), the funcs arrays are dynamically allocat=
ed
> for each object in a loop:
>
>         funcs =3D kzalloc(sizeof(struct klp_func) * (nr_funcs + 1), GFP_K=
ERNEL);
>         ...
>         obj->funcs =3D funcs;
>
> If KLP_OBSOLETES parsing fails during string duplication, array allocatio=
n,
> or integer conversion, the error paths jump to err_free_objs. It appears
> that the cleanup label only frees the objs array itself but leaves the
> individual obj->funcs arrays allocated:
>
> err_free_objs:
>         kfree(objs);
> err_free_patch:
>         kfree(patch);

You are right.
Furthermore, there is a pre-existing bug in the error path that should
be fixed as well:

--- a/scripts/livepatch/init.c
+++ b/scripts/livepatch/init.c
@@ -52,7 +52,7 @@ static int __init livepatch_mod_init(void)
                if (!funcs) {
                        ret =3D -ENOMEM;
                        for (int j =3D 0; j < i; j++)
-                               kfree(objs[i].funcs);
+                               kfree(objs[j].funcs);
                        goto err_free_objs;
                }

--=20
Regards
Yafang