On Thu, Jan 5, 2023 at 7:05 AM Joe Lawrence <joe.lawrence@xxxxxxxxxx> wrote: > > On 1/5/23 00:59, Song Liu wrote: > > On Wed, Jan 4, 2023 at 3:12 PM Joe Lawrence <joe.lawrence@xxxxxxxxxx> wrote: > >> > >> > >> Stepping back, this feature is definitely foot-gun capable. > >> Kpatch-build would expect that klp-relocations would only ever be needed > >> in code that will patch the very same module that provides the > >> relocation destination -- that is, it was never intended to reference > >> through one of these klp-relocations unless it resolved to a live > >> module. > >> > >> On the other hand, when writing the selftests, verifying against NULL > >> [1] provided 1) a quick sanity check that something was "cleared" and 2) > >> protected the machine against said foot-gun. > >> > >> [1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c > > > > I don't quite follow the foot-gun here. What's the failure mode? > > > > Kpatch-build, for better or worse, hides the potential problem. A > typical kpatch scenario would be: > > 1. A patch modifies module foo's function bar(), which references > symbols local to module foo > > 2. Kpatch-build creates a livepatch .ko with klp-relocations in the > modified bar() to foo's symbols > > 3. When loaded, modified bar() code that references through its > klp-relocations to module foo will only ever be active when foo is > loaded, i.e. when the original bar() redirects to the livepatch version. > > However, writing source-based livepatches (like the kselftests) offers a > lot more freedom. There is no implicit guarantee from (3) that the > module is loaded. One could reference klp-relocations from anywhere in > the livepatch module. > > For example, in my test_klp_convert1.c test case, I have a livepatch > module with a sysfs interface (print_debug_set()) that allows the test > bash script to force the module to references through its > klp-relocations (print_static_strings()): > > ... > static void print_string(const char *label, const char *str) > { > if (str) > pr_info("%s: %s\n", label, str); > } > ... > static noinline void print_static_strings(void) > { > print_string("klp_string.12345", klp_string_a); > print_string("klp_string.67890", klp_string_b); > } > > /* provide a sysfs handle to invoke debug functions */ > static int print_debug; > static int print_debug_set(const char *val, const struct kernel_param *kp) > { > print_static_strings(); > > return 0; > } > static const struct kernel_param_ops print_debug_ops = { > .set = print_debug_set, > .get = param_get_int, > }; > > > When I first wrote test_klp_convert1.c, I did not have wrappers like > print_string(), I simply referenced the symbols directly and send them > to pr_info as "%s" string formatting options. > > You can probably see where this is going when I unloaded the module that > provided klp_string_a, klp_string_b, etc. and invoked the sysfs handle. > The stale relocation values point to ... somewhere we shouldn't try to > reference any more. Thanks for the explanation. > Perhaps I'm too paranoid about that possibility, but by actually > clearing the values in the relocations on module removal, one could > check them and try to guard against dangling pointer (dangling > relocation?) references. I personally don't worry too much about this issue. But clearing the relocations seems to be a good idea. Thanks, Song
