Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 18 Apr 2022 10:04:42 +0530
Message-ID: <747830777.1512786.1650256482294@mail-kr5-3>
Mime-Version: 1.0
Subject: RE: [PATCH 1/1] kallsyms: add kallsyms_show_value definition in all
 cases
Reply-To: maninder1.s@samsung.com
Sender: Maninder Singh <maninder1.s@samsung.com>
From:   Maninder Singh <maninder1.s@samsung.com>
To:     Petr Mladek <pmladek@suse.com>,
        "mcgrof@kernel.org" <mcgrof@kernel.org>
CC:     "rostedt@goodmis.org" <rostedt@goodmis.org>,
        "senozhatsky@chromium.org" <senozhatsky@chromium.org>,
        "andriy.shevchenko@linux.intel.com" 
        <andriy.shevchenko@linux.intel.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "wangkefeng.wang@huawei.com" <wangkefeng.wang@huawei.com>,
        "wedsonaf@google.com" <wedsonaf@google.com>,
        "boqun.feng@gmail.com" <boqun.feng@gmail.com>,
        "christophe.leroy@csgroup.eu" <christophe.leroy@csgroup.eu>,
        "swboyd@chromium.org" <swboyd@chromium.org>,
        "ojeda@kernel.org" <ojeda@kernel.org>,
        "ast@kernel.org" <ast@kernel.org>,
        "gary@garyguo.net" <gary@garyguo.net>,
        "mbenes@suse.cz" <mbenes@suse.cz>,
        "ndesaulniers@google.com" <ndesaulniers@google.com>,
        "void@manifault.com" <void@manifault.com>,
        "jolsa@kernel.org" <jolsa@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Vaneet Narang <v.narang@samsung.com>,
        Onkarnath <onkarnath.1@samsung.com>
In-Reply-To: <YlaWxh5qYCe40US7@alley>
Content-Type: multipart/mixed;
        boundary="----=_Part_1512785_2026329720.1650256482294"
CMS-TYPE: 105P
References: <YlaWxh5qYCe40US7@alley>
        <20220413055305.1768223-1-maninder1.s@samsung.com>
        <CGME20220413055318epcas5p3df3cdde54a559d4002a74de9f23289f2@epcms5p5>
Precedence: bulk

------=_Part_1512785_2026329720.1650256482294
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"

Hi,

> 
> With the patch, kallsyms_show_value() might return true even when
> CONFIG_KALLYSMS are disabled. Did you check all users of this API
> that they could handle this?
> 
> For example, the comment in bpf_dump_raw_ok() suggests that
> it might require more kallsyms functionality.
>

Ok, we will check these other callers also, if it causes negative impact at other places.

> static inline bool bpf_dump_raw_ok(const struct cred *cred)
> {
> 	/* Reconstruction of call-sites is dependent on kallsyms,
> 	 * thus make dump the same restriction.
> 	 */
> 	return kallsyms_show_value(cred);
> }
> 
> You should definitely add into CC people from affected subsystems.
> I do not see:
> 
>     + Kees Cook who added/updated most of the callers
>     + BPF people that might require even more kallsyms functionality
>     + kprobe people that using it as well
>

Yes, I thought maintainer.pl has shown all people, but seems caller function owners
also need to be added, I will add  in next revision.


> > +/*
> > + * We show kallsyms information even to normal users if we've enabled
> > + * kernel profiling and are explicitly not paranoid (so kptr_restrict
> > + * is clear, and sysctl_perf_event_paranoid isn't set).
> > + *
> > + * Otherwise, require CAP_SYSLOG (assuming kptr_restrict isn't set to
> > + * block even that).
> > + */
> > +bool kallsyms_show_value(const struct cred *cred)
> > +{
> > +	switch (kptr_restrict) {
> > +	case 0:
> > +		if (kallsyms_for_perf())
> > +			return true;
> > +		fallthrough;
> > +	case 1:
> > +		if (security_capable(cred, &init_user_ns, CAP_SYSLOG,
> > +				     CAP_OPT_NOAUDIT) == 0)
> > +			return true;
> > +		fallthrough;
> > +	default:
> > +		return false;
> > +	}
> > +}
> 
> It is really weird that the function is declared in kallsyms.h
> and implemented in vsprintf.c.
>

Yes it does not look good.
Initially we thought to make it as static inline function in kallsyms.h only.
But as function is little big and it will increase code size, so we added
definition in vsprintf.c, because its alwyas compilable code and also it has
some wrapper APIs for kallsyms.
 
But as you suggested to make a new file, it will be good.

> What about splitting kallsyms.c into two source files where one
> would be always compiled? It would be usable also for the
> spring function introduced by
> https://lore.kernel.org/r/20220323164742.2984281-1-maninder1.s@samsung.com
> 
> It might be something like kallsyms_full.c and/or kallsyms_tiny.c.
>

This patch is not taken by Luis yet in module-tetsing branch.
So what will be the best approach to make new version of this patch ?

A) to make new file kallsyms_tiny.c and add only one definition in that file
and when above patch of spring functions is merged in next we can move definitions
to new file

or

B) we send patch to Luis's branch of module-testing with moving definition(of earlier patch)
to new file and current patch also.

Thanks,
Maninder Singh

------=_Part_1512785_2026329720.1650256482294
Content-Type: application/octet-stream
Content-Disposition: attachment; filename="rcptInfo.txt"
Content-Transfer-Encoding: base64

DQogICA9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT0NCiAgICAgIFN1YmplY3QgICAgOiBSZTogW1BBVENIIDEvMV0g
a2FsbHN5bXM6IGFkZCBrYWxsc3ltc19zaG93X3ZhbHVlIGRlZmluaXRpb24gaW4gYWxsIGNhc2Vz
DQogICAgICBGcm9tICAgICAgIDogbnVsbA0KICAgICAgU2VudCBEYXRlICA6IDIwMjItMDQtMTMg
MTQ6NTQgIEdNVCs1OjMwDQogICA9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCiAgICAgICAgICAgICAgICAgIE5h
bWUgICAgICAgICAgICAgICAgVHlwZSAgICAgICAgICBKb2IgVGl0bGUgICAgICAgICAgICAgICAg
ICAgICAgIERlcHQuICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIENvbXBhbnkgICAgICAg
ICAgICAgICAgDQogICA9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCiAgICAgIE1hbmluZGVyIFNpbmdoICAgICAg
ICAgICAgICAgICBUTyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIFN5c3RlbSBT
L1cgR3JvdXAgL1NSSS1EZWxoaSAgICAgICAgICAgICAgIFNhbXN1bmfCoEVsZWN0cm9uaWNzwqAN
CiAgICAgIG1jZ3JvZkBrZXJuZWwub3JnICAgICAgICAgICAgICBDQw0KICAgICAgcm9zdGVkdEBn
b29kbWlzLm9yZyAgICAgICAgICAgIENDDQogICAgICBzZW5vemhhdHNreUBjaHJvbWl1bS5vcmcg
ICAgICAgQ0MNCiAgICAgIGFuZHJpeS5zaGV2Y2hlbmtvQGxpbnV4LmkuLi4gICBDQw0KICAgICAg
YWtwbUBsaW51eC1mb3VuZGF0aW9uLm9yZyAgICAgIENDDQogICAgICB3YW5na2VmZW5nLndhbmdA
aHVhd2VpLmNvbSAgICAgQ0MNCiAgICAgIHdlZHNvbmFmQGdvb2dsZS5jb20gICAgICAgICAgICBD
Qw0KICAgICAgYm9xdW4uZmVuZ0BnbWFpbC5jb20gICAgICAgICAgIENDDQogICAgICBjaHJpc3Rv
cGhlLmxlcm95QGNzZ3JvdXAuZXUgICAgQ0MNCiAgICAgIHN3Ym95ZEBjaHJvbWl1bS5vcmcgICAg
ICAgICAgICBDQw0KICAgICAgb2plZGFAa2VybmVsLm9yZyAgICAgICAgICAgICAgIENDDQogICAg
ICBhc3RAa2VybmVsLm9yZyAgICAgICAgICAgICAgICAgQ0MNCiAgICAgIGdhcnlAZ2FyeWd1by5u
ZXQgICAgICAgICAgICAgICBDQw0KICAgICAgbWJlbmVzQHN1c2UuY3ogICAgICAgICAgICAgICAg
IENDDQogICAgICBuZGVzYXVsbmllcnNAZ29vZ2xlLmNvbSAgICAgICAgQ0MNCiAgICAgIHZvaWRA
bWFuaWZhdWx0LmNvbSAgICAgICAgICAgICBDQw0KICAgICAgam9sc2FAa2VybmVsLm9yZyAgICAg
ICAgICAgICAgIENDDQogICAgICBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnICAgQ0MNCiAg
ICAgIFZhbmVldCBOYXJhbmcgICAgICAgICAgICAgICAgICBDQyAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgIFN5c3RlbSBTL1cgR3JvdXAgL1NSSS1EZWxoaSAgICAgICAgICAgICAg
IFNhbXN1bmcgRWxlY3Ryb25pY3MNCiAgICAgIE9ua2FybmF0aCAgICAgICAgICAgICAgICAgICAg
ICBDQyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIFN5c3RlbSBTL1cgR3JvdXAg
L1NSSS1EZWxoaSAgICAgICAgICAgICAgIFNhbXN1bmcgRWxlY3Ryb25pY3MNCiAgID09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PQ0K

------=_Part_1512785_2026329720.1650256482294--