Re: [PATCH 1/4] kunit: Fix wild-memory-access bug in kunit_free_suite_set()

[David Gow <davidgow@google.com>]

On Thu, 31 Aug 2023 at 15:17, 'Jinjie Ruan' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> Inject fault while probing kunit-example-test.ko, if kstrdup()
> fails in mod_sysfs_setup() in load_module(), the mod->state will
> switch from MODULE_STATE_COMING to MODULE_STATE_GOING instead of
> from MODULE_STATE_LIVE to MODULE_STATE_GOING, so only
> kunit_module_exit() will be called without kunit_module_init(), and
> the mod->kunit_suites is no set correctly and the free in
> kunit_free_suite_set() will cause below wild-memory-access bug.
>
> The mod->state state machine when load_module() succeeds:
>
> MODULE_STATE_UNFORMED ---> MODULE_STATE_COMING ---> MODULE_STATE_LIVE
>          ^                                              |
>          |                                              | delete_module
>          +---------------- MODULE_STATE_GOING <---------+
>
> The mod->state state machine when load_module() fails at
> mod_sysfs_setup():
>
> MODULE_STATE_UNFORMED ---> MODULE_STATE_COMING ---> MODULE_STATE_GOING
>         ^                                               |
>         |                                               |
>         +-----------------------------------------------+
>
> Call kunit_module_init() at MODULE_STATE_COMING state to fix the issue
> because MODULE_STATE_LIVE is transformed from it.
>
>  Unable to handle kernel paging request at virtual address ffffff341e942a88
>  KASAN: maybe wild-memory-access in range [0x0003f9a0f4a15440-0x0003f9a0f4a15447]
>  Mem abort info:
>    ESR = 0x0000000096000004
>    EC = 0x25: DABT (current EL), IL = 32 bits
>    SET = 0, FnV = 0
>    EA = 0, S1PTW = 0
>    FSC = 0x04: level 0 translation fault
>  Data abort info:
>    ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
>    CM = 0, WnR = 0, TnD = 0, TagAccess = 0
>    GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
>  swapper pgtable: 4k pages, 48-bit VAs, pgdp=00000000441ea000
>  [ffffff341e942a88] pgd=0000000000000000, p4d=0000000000000000
>  Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
>  Modules linked in: kunit_example_test(-) cfg80211 rfkill 8021q garp mrp stp llc ipv6 [last unloaded: kunit_example_test]
>  CPU: 3 PID: 2035 Comm: modprobe Tainted: G        W        N 6.5.0-next-20230828+ #136
>  Hardware name: linux,dummy-virt (DT)
>  pstate: a0000005 (NzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>  pc : kfree+0x2c/0x70
>  lr : kunit_free_suite_set+0xcc/0x13c
>  sp : ffff8000829b75b0
>  x29: ffff8000829b75b0 x28: ffff8000829b7b90 x27: 0000000000000000
>  x26: dfff800000000000 x25: ffffcd07c82a7280 x24: ffffcd07a50ab300
>  x23: ffffcd07a50ab2e8 x22: 1ffff00010536ec0 x21: dfff800000000000
>  x20: ffffcd07a50ab2f0 x19: ffffcd07a50ab2f0 x18: 0000000000000000
>  x17: 0000000000000000 x16: 0000000000000000 x15: ffffcd07c24b6764
>  x14: ffffcd07c24b63c0 x13: ffffcd07c4cebb94 x12: ffff700010536ec7
>  x11: 1ffff00010536ec6 x10: ffff700010536ec6 x9 : dfff800000000000
>  x8 : 00008fffefac913a x7 : 0000000041b58ab3 x6 : 0000000000000000
>  x5 : 1ffff00010536ec5 x4 : ffff8000829b7628 x3 : dfff800000000000
>  x2 : ffffff341e942a80 x1 : ffffcd07a50aa000 x0 : fffffc0000000000
>  Call trace:
>   kfree+0x2c/0x70
>   kunit_free_suite_set+0xcc/0x13c
>   kunit_module_notify+0xd8/0x360
>   blocking_notifier_call_chain+0xc4/0x128
>   load_module+0x382c/0x44a4
>   init_module_from_file+0xd4/0x128
>   idempotent_init_module+0x2c8/0x524
>   __arm64_sys_finit_module+0xac/0x100
>   invoke_syscall+0x6c/0x258
>   el0_svc_common.constprop.0+0x160/0x22c
>   do_el0_svc+0x44/0x5c
>   el0_svc+0x38/0x78
>   el0t_64_sync_handler+0x13c/0x158
>   el0t_64_sync+0x190/0x194
>  Code: aa0003e1 b25657e0 d34cfc42 8b021802 (f9400440)
>  ---[ end trace 0000000000000000 ]---
>  Kernel panic - not syncing: Oops: Fatal exception
>  SMP: stopping secondary CPUs
>  Kernel Offset: 0x4d0742200000 from 0xffff800080000000
>  PHYS_OFFSET: 0xffffee43c0000000
>  CPU features: 0x88000203,3c020000,1000421b
>  Memory Limit: none
>  Rebooting in 1 seconds..
>
> Fixes: 3d6e44623841 ("kunit: unify module and builtin suite definitions")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---

Nice catch, thanks. I'm not seeing the actual panic here (with a quick
hack to fail module loading), but the patch looks sound.

I suspect we may want to split the module loading up into separate
'init' and 'run' phases, tied to MODULE_STATE_COMING and
MODULE_STATE_LIVE respectively, at some point (perhaps when the
planned support for triggering tests from userspace is done), but this
is a good fix in the meantime,

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David


>  lib/kunit/test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 49698a168437..421f13981412 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -784,12 +784,13 @@ static int kunit_module_notify(struct notifier_block *nb, unsigned long val,
>
>         switch (val) {
>         case MODULE_STATE_LIVE:
> -               kunit_module_init(mod);
>                 break;
>         case MODULE_STATE_GOING:
>                 kunit_module_exit(mod);
>                 break;
>         case MODULE_STATE_COMING:
> +               kunit_module_init(mod);
> +               break;
>         case MODULE_STATE_UNFORMED:
>                 break;
>         }
> --
> 2.34.1
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230831071655.2907683-2-ruanjinjie%40huawei.com.