On Tue 2026-09-08 12:18:10, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [Medium] Off-by-one error in the rollback loop leaves the failing section's
> partially applied relocations uncleared.
> --
>
> commit d297505bb4b1d833ec93725770d65ac1044abcad
> Author: Petr Mladek <[email protected]>
>
> livepatch: Clear relocations when klp_apply_object_relocs() fails
>
> This commit introduces klp_write_object_relocs_limited() to unwind and
> clear only the relocations that were successfully applied before
> klp_write_object_relocs() encountered an error. This prevents partially
> applied patches from leaving un-reverted changes that cause subsequent
> patch operations to fail validation.
>
> Link: https://lore.kernel.org/r/[email protected]
>
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index a6796cd6b65fd..714f97fdd271a 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -342,14 +342,17 @@ int klp_apply_section_relocs(struct module *pmod,
> > Elf_Shdr *sechdrs,
> [ ... ]
> > +static int klp_write_object_relocs_limited(struct klp_patch *patch,
> > + struct klp_object *obj,
> > + bool apply, int limit)
> > {
> > int i, ret;
> > struct klp_modinfo *info = patch->mod->klp_info;
> >
> > - for (i = 1; i < info->hdr.e_shnum; i++) {
> > + if (!limit || limit > info->hdr.e_shnum)
> > + limit = info->hdr.e_shnum;
> > +
> > + for (i = 1; i < limit; i++) {
> > Elf_Shdr *sec = info->sechdrs + i;
> [ ... ]
> > - if (ret)
> > + if (ret) {
> > + if (apply)
> > + klp_write_object_relocs_limited(patch, obj,
> > false, i);
>
> [Severity: Medium]
> When an error occurs in klp_write_section_relocs(), the rollback is triggered
> with the failing section index passed as the limit. Since the rollback loop
> iterates with a strict less-than bounds check against this limit, will this
> skip the rollback for any relocations that were successfully applied within
> the failing section itself before it encountered the error?
>
> It looks like this could leave partially applied relocations permanently
> in memory within the failing section, causing future module loads to fail
> validation.
Sigh, I did not go deep enough. And it goes down to the arch-specific
implementations, in particular arch/x86/kernel/module.c:
static int __write_relocate_add(Elf64_Shdr *sechdrs,
const char *strtab,
unsigned int symindex,
unsigned int relsec,
struct module *me,
void *(*write)(void *dest, const void *src, size_t len),
bool apply)
{
[...]
for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
[...]
if (memcmp(loc, &zero, size)) {
pr_err("x86/modules: Invalid relocation target,
existing value is nonzero for sec %u, idx %u, type %d, loc %lx, val %llx\n",
relsec, i,
(int)ELF64_R_TYPE(rel[i].r_info),
(unsigned long)loc, val);
return -ENOEXEC;
}
write(loc, &val, size);
} else {
if (memcmp(loc, &val, size)) {
pr_warn("x86/modules: Invalid relocation
target, existing value does not match expected value for sec %u, idx %u, type
%d, loc %lx, val %llx\n",
relsec, i,
(int)ELF64_R_TYPE(rel[i].r_info),
(unsigned long)loc, val);
return -ENOEXEC;
}
write(loc, &zero, size);
}
[...]
We would need to implement the revert at this level.
IMHO, it could be done separately.
I used Gemini LLM to check it and it seems that x86_64 is the only
architecture with these permissive checks. It seems that all other
architectures just write the new value without checking
the existing one.
Would you go this way and try to get patch into x86 code?
Should I do it in v5 or separately?
Best Regards,
Petr
PS: I would prefer to go with unless it causes regression
and fix this "later". We could not endlessly delay
fixes because of Sashiko finding pre-existing problems.
> > return ret;
> > + }
> > }
> >
> > return 0;
> > }
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/[email protected]?part=4
