linux-kernel - Re: [PATCH] sysctl: unregister sysctl table after testing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5cves6b5bzpvmhwl3jqepz4pm5oz226fzjqzlxpzmxvxmiftv2@2vgc37lywq3a>
Date: Wed, 12 Mar 2025 22:15:45 +0100
From: Joel Granados <joel.granados@...nel.org>
To: John Sperbeck <jsperbeck@...gle.com>
Cc: Kees Cook <kees@...nel.org>, Wen Yang <wen.yang@...ux.dev>, 
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] sysctl: unregister sysctl table after testing

On Mon, Jan 06, 2025 at 03:15:13PM +0100, Joel Granados wrote:
> 
> On Tue, Dec 24, 2024 at 09:11:24AM -0800, John Sperbeck wrote:
> > In commit b5ffbd139688 ("sysctl: move the extra1/2 boundary check
> > of u8 to sysctl_check_table_array"), a kunit test was added that
> > registers a sysctl table.  If the test is run as a module, then a
> > lingering reference to the module is left behind, and a 'sysctl -a'
> > leads to a panic.
> 
> Very good catch indeed!!!.
> > 
...
> >  	KUNIT_EXPECT_NULL(test, register_sysctl("foo", table_foo));
> >  	KUNIT_EXPECT_NULL(test, register_sysctl("foo", table_bar));
> > -	KUNIT_EXPECT_NOT_NULL(test, register_sysctl("foo", table_qux));
> > +	hdr = register_sysctl("foo", table_qux);
> > +	KUNIT_EXPECT_NOT_NULL(test, hdr);
> > +	unregister_sysctl_table(hdr);
> This indeed fixes the behaviour, but it is not what should be done
> and this is why:
> 1. sysctl-test.c is part of the unit tests for sysctl and actually
>    trying to execute a register here does not really make sense.
> 2. The file that actually does the regression testing is
>    lib/test_sysctl.c
> 
> If you are up for it this is what needs to be done:
> 1. change what is in sysctl-test.c to call sysctl_check_table_array
>    directly and not worry about keeping track of the registration.

With your V4 it is clear to me know that I should *not* have made the
first suggestion of calling sysctl_check_table. Exposing the
sysctl_check_table just for the kunit test is overkill as we can get the
same result with a register call from lib/tests_sysctl.c without all the
hassle of exposing the function call. Your proposal was still valuable
as it clarified what the "right" approach should be.

Best
> 2. Add a similar regression test in lib/test_sysctl.c where we actually
>    check for the error.
> 
> Please tell me if you are up for it (if not I can add it to my todos)
> 
> Best
> 
> >  }
> >  
> >  static struct kunit_case sysctl_test_cases[] = {
> > -- 
> > 2.47.1.613.gc27f4b7a9f-goog
> > 
> 
> -- 
> 
> Joel Granados

-- 

Joel Granados