Re: [PATCH v2 2/2] usercopy: Convert test_user_copy to KUnit test

Re: [PATCH v2 2/2] usercopy: Convert test_user_copy to KUnit test

[Kees Cook]

On Wed, Jun 12, 2024 at 05:13:39PM +0800, David Gow wrote:
> On Tue, 11 Jun 2024 at 05:33, Kees Cook <kees@kernel.org> wrote:
> >
> > Convert the runtime tests of hardened usercopy to standard KUnit tests.
> >
> > Co-developed-by: Vitor Massaru Iha <vitor@massaru.org>
> > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org>
> > Link: https://lore.kernel.org/r/20200721174654.72132-1-vitor@massaru.org
> > Tested-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> > Signed-off-by: Kees Cook <kees@kernel.org>
> > ---
> 
> This looks good, particularly with the x86 fix applied.
> 
> It's still hanging on m68k -- I think at the 'illegal reversed
> copy_to_user passed' test -- but I'll admit to not having tried to
> debug it further.
> 
> One other (set of) notes below about using KUNIT_EXPECT_MEMEQ_MSG(),
> otherwise (assuming the m68k stuff isn't actually a regression, which
> I haven't tested but I imagine is unlikely),

Hi Geert,

I'm trying to debug a hang on m68k in the usercopy behavioral testing
routines. It's testing for the pathological case of having inverted
arguments to copy_to_user():

        user_addr = kunit_vm_mmap(test, NULL, 0, priv->size,
                            PROT_READ | PROT_WRITE | PROT_EXEC,
                            MAP_ANONYMOUS | MAP_PRIVATE, 0);
	...
	bad_usermem = (char *)user_addr;
	...
	KUNIT_EXPECT_NE_MSG(test, copy_to_user((char __user *)kmem, bad_usermem,
					       PAGE_SIZE), 0,
		"illegal reversed copy_to_user passed");

On other architectures, this immediate fails because the access_ok()
check rejects it. On m68k with CONFIG_ALTERNATE_USER_ADDRESS_SPACE,
access_ok() short-circuits to "true". I've tried reading
arch/m68k/include/asm/uaccess.h but I'm not sure what's happening under
CONFIG_CPU_HAS_ADDRESS_SPACES.

For now I've excluded that test for m68k, but I'm not sure what's
expected to happen here on m68k for this set of bad arguments. Can you
advise?

Thanks!

-Kees

-- 
Kees Cook

Re: [PATCH v2 2/2] usercopy: Convert test_user_copy to KUnit test

[Geert Uytterhoeven]

Hi Kees,

On Wed, Jun 12, 2024 at 6:51 PM Kees Cook <kees@kernel.org> wrote:
> On Wed, Jun 12, 2024 at 05:13:39PM +0800, David Gow wrote:
> > On Tue, 11 Jun 2024 at 05:33, Kees Cook <kees@kernel.org> wrote:
> > > Convert the runtime tests of hardened usercopy to standard KUnit tests.
> > >
> > > Co-developed-by: Vitor Massaru Iha <vitor@massaru.org>
> > > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org>
> > > Link: https://lore.kernel.org/r/20200721174654.72132-1-vitor@massaru.org
> > > Tested-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> > > Signed-off-by: Kees Cook <kees@kernel.org>
> > > ---
> >
> > This looks good, particularly with the x86 fix applied.
> >
> > It's still hanging on m68k -- I think at the 'illegal reversed
> > copy_to_user passed' test -- but I'll admit to not having tried to
> > debug it further.
> >
> > One other (set of) notes below about using KUNIT_EXPECT_MEMEQ_MSG(),
> > otherwise (assuming the m68k stuff isn't actually a regression, which
> > I haven't tested but I imagine is unlikely),
>
> I'm trying to debug a hang on m68k in the usercopy behavioral testing
> routines. It's testing for the pathological case of having inverted
> arguments to copy_to_user():
>
>         user_addr = kunit_vm_mmap(test, NULL, 0, priv->size,
>                             PROT_READ | PROT_WRITE | PROT_EXEC,
>                             MAP_ANONYMOUS | MAP_PRIVATE, 0);
>         ...
>         bad_usermem = (char *)user_addr;
>         ...
>         KUNIT_EXPECT_NE_MSG(test, copy_to_user((char __user *)kmem, bad_usermem,
>                                                PAGE_SIZE), 0,
>                 "illegal reversed copy_to_user passed");
>
> On other architectures, this immediate fails because the access_ok()
> check rejects it. On m68k with CONFIG_ALTERNATE_USER_ADDRESS_SPACE,
> access_ok() short-circuits to "true". I've tried reading
> arch/m68k/include/asm/uaccess.h but I'm not sure what's happening under
> CONFIG_CPU_HAS_ADDRESS_SPACES.

On m68k CPUs that support CPU_HAS_ADDRESS_SPACES (i.e. all traditional
680x0 that can run real Linux), the CPU has separate address spaces
for kernel and user addresses.  Accessing userspace addresses is done
using the special "moves" instruction, so we can just use the MMU to
catch invalid accesses.

> For now I've excluded that test for m68k, but I'm not sure what's
> expected to happen here on m68k for this set of bad arguments. Can you
> advise?

Perhaps the kernel address is actually a valid user address, or
vice versa?

Does the test work on systems that use 4G/4G for kernel/userspace
instead of the usual 1G/3G split?

/me runs the old test_user_copy.ko on ARAnyM
Seems to take a while? Or it hangs, too?

Related reading material
https://lore.kernel.org/all/CAMuHMdUzHwm5_TL7TNAOF+uqheJnKgsqF+_vzqGRzB_3eufKug@mail.gmail.com/
https://lore.kernel.org/all/CAMuHMdVQ93ihgcxUbjptTaHdPjxXLyVAsAr-m3tWBJV0krS2vw@mail.gmail.com/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] usercopy: Convert test_user_copy to KUnit test

[Kees Cook]

On Wed, Jun 12, 2024 at 09:21:52PM +0200, Geert Uytterhoeven wrote:
> Hi Kees,
> 
> On Wed, Jun 12, 2024 at 6:51 PM Kees Cook <kees@kernel.org> wrote:
> > On Wed, Jun 12, 2024 at 05:13:39PM +0800, David Gow wrote:
> > > On Tue, 11 Jun 2024 at 05:33, Kees Cook <kees@kernel.org> wrote:
> > > > Convert the runtime tests of hardened usercopy to standard KUnit tests.
> > > >
> > > > Co-developed-by: Vitor Massaru Iha <vitor@massaru.org>
> > > > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org>
> > > > Link: https://lore.kernel.org/r/20200721174654.72132-1-vitor@massaru.org
> > > > Tested-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> > > > Signed-off-by: Kees Cook <kees@kernel.org>
> > > > ---
> > >
> > > This looks good, particularly with the x86 fix applied.
> > >
> > > It's still hanging on m68k -- I think at the 'illegal reversed
> > > copy_to_user passed' test -- but I'll admit to not having tried to
> > > debug it further.
> > >
> > > One other (set of) notes below about using KUNIT_EXPECT_MEMEQ_MSG(),
> > > otherwise (assuming the m68k stuff isn't actually a regression, which
> > > I haven't tested but I imagine is unlikely),
> >
> > I'm trying to debug a hang on m68k in the usercopy behavioral testing
> > routines. It's testing for the pathological case of having inverted
> > arguments to copy_to_user():
> >
> >         user_addr = kunit_vm_mmap(test, NULL, 0, priv->size,
> >                             PROT_READ | PROT_WRITE | PROT_EXEC,
> >                             MAP_ANONYMOUS | MAP_PRIVATE, 0);
> >         ...
> >         bad_usermem = (char *)user_addr;
> >         ...
> >         KUNIT_EXPECT_NE_MSG(test, copy_to_user((char __user *)kmem, bad_usermem,
> >                                                PAGE_SIZE), 0,
> >                 "illegal reversed copy_to_user passed");
> >
> > On other architectures, this immediate fails because the access_ok()
> > check rejects it. On m68k with CONFIG_ALTERNATE_USER_ADDRESS_SPACE,
> > access_ok() short-circuits to "true". I've tried reading
> > arch/m68k/include/asm/uaccess.h but I'm not sure what's happening under
> > CONFIG_CPU_HAS_ADDRESS_SPACES.
> 
> On m68k CPUs that support CPU_HAS_ADDRESS_SPACES (i.e. all traditional
> 680x0 that can run real Linux), the CPU has separate address spaces
> for kernel and user addresses.  Accessing userspace addresses is done
> using the special "moves" instruction, so we can just use the MMU to
> catch invalid accesses.

Okay, that's what I suspected. I think I'll need to just not test this
particular case for archs with separate address spaces, since it would
be meaningless.

> 
> > For now I've excluded that test for m68k, but I'm not sure what's
> > expected to happen here on m68k for this set of bad arguments. Can you
> > advise?
> 
> Perhaps the kernel address is actually a valid user address, or
> vice versa?

Right -- I think that's what's happened.

> 
> Does the test work on systems that use 4G/4G for kernel/userspace
> instead of the usual 1G/3G split?
> 
> /me runs the old test_user_copy.ko on ARAnyM
> Seems to take a while? Or it hangs, too?

Sounds like the same behavior.

> Related reading material
> https://lore.kernel.org/all/CAMuHMdUzHwm5_TL7TNAOF+uqheJnKgsqF+_vzqGRzB_3eufKug@mail.gmail.com/
> https://lore.kernel.org/all/CAMuHMdVQ93ihgcxUbjptTaHdPjxXLyVAsAr-m3tWBJV0krS2vw@mail.gmail.com/

Thanks!

-Kees

-- 
Kees Cook