linux-kernel - Re: [PATCH v4] modpost: check for static EXPORT

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: <e206f4ff-1fbe-68e4-3614-85f4cbc0223f@linux.com>
Date:   Tue, 30 Jul 2019 21:15:56 +0300
From:   Denis Efremov <efremov@...ux.com>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Michal Marek <michal.lkml@...kovi.net>,
        Emil Velikov <emil.l.velikov@...il.com>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] modpost: check for static EXPORT_SYMBOL* functions

Changes in v1:
1. Fixed indentations.
2. Removed lkml links from the description of the commit.
Changes in v2:
1. Changed the 'n' variable type in the main function from size_t to
    int.
Changes in v3:
1. Changed warning message from "%s is the static EXPORT_*" to
    "%s a static ..."
2. Improved the commit description from "approx. 1 min" to
    "less than a minute". Thanks Stephen Rothwell for additional
    measurements.
Changes in v4:
1. Dropped ELF_ST_TYPE check. This fixes false-positives detected by
    Stephen Rothwell.
2. Moved ELF_ST_BIND check before the call to find_symbol. Thanks
    Masahiro Yamada for suggesting it.

Denis

On 30.07.2019 21:11, Denis Efremov wrote:
> This patch adds a check to warn about static EXPORT_SYMBOL* functions
> during the modpost. In most of the cases, a static symbol marked for
> exporting is an odd combination that should be fixed either by deleting
> the exporting mark or by removing the static attribute and adding the
> appropriate declaration to headers.
> 
> This check could help to detect the following problems:
> 1. 550113d4e9f5 ("i2c: add newly exported functions to the header, too")
> 2. 54638c6eaf44 ("net: phy: make exported variables non-static")
> 3. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
> 4. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration")
> 5. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
> 6. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next")
> 7. 14b4c48bb1ce ("gve: Remove the exporting of gve_probe")
> 8. 9b79ee9773a8 ("scsi: libsas: remove the exporting of sas_wait_eh")
> 9. ...
> 
> Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):
> $ make mrproper; make allmodconfig; time make -j12; \
>    git checkout HEAD~1; \
>    make mrproper; make allmodconfig; time make -j12
> 1.
>     (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
>     (w/o  patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
> 2.
>     (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total
>     (w/o  patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
> 3.
>     (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
>     (w/o  patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total
> 
> The check adds less than a minute to a usual build.
> 
> Acked-by: Emil Velikov <emil.l.velikov@...il.com>
> Signed-off-by: Denis Efremov <efremov@...ux.com>
> ---
>   scripts/mod/modpost.c | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
> 
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index f277e116e0eb..3e6d36ddfcdf 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -169,6 +169,7 @@ struct symbol {
>   	unsigned int kernel:1;     /* 1 if symbol is from kernel
>   				    *  (only for external modules) **/
>   	unsigned int preloaded:1;  /* 1 if symbol from Module.symvers, or crc */
> +	unsigned int is_static:1;  /* 1 if symbol is not global */
>   	enum export  export;       /* Type of export */
>   	char name[0];
>   };
> @@ -201,6 +202,7 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak,
>   	strcpy(s->name, name);
>   	s->weak = weak;
>   	s->next = next;
> +	s->is_static = 1;
>   	return s;
>   }
>   
> @@ -1980,6 +1982,21 @@ static void read_symbols(const char *modname)
>   		handle_modversions(mod, &info, sym, symname);
>   		handle_moddevtable(mod, &info, sym, symname);
>   	}
> +
> +	// check for static EXPORT_SYMBOL_* functions && global vars
> +	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
> +		unsigned char bind = ELF_ST_BIND(sym->st_info);
> +
> +		if (bind == STB_GLOBAL || bind == STB_WEAK) {
> +			struct symbol *s =
> +				find_symbol(remove_dot(info.strtab +
> +						       sym->st_name));
> +
> +			if (s)
> +				s->is_static = 0;
> +		}
> +	}
> +
>   	if (!is_vmlinux(modname) || vmlinux_section_warnings)
>   		check_sec_ref(mod, modname, &info);
>   
> @@ -2425,6 +2442,7 @@ int main(int argc, char **argv)
>   	char *dump_write = NULL, *files_source = NULL;
>   	int opt;
>   	int err;
> +	int n;
>   	struct ext_sym_list *extsym_iter;
>   	struct ext_sym_list *extsym_start = NULL;
>   
> @@ -2520,6 +2538,19 @@ int main(int argc, char **argv)
>   	if (sec_mismatch_count && sec_mismatch_fatal)
>   		fatal("modpost: Section mismatches detected.\n"
>   		      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
> +	for (n = 0; n < SYMBOL_HASH_SIZE; n++) {
> +		struct symbol *s = symbolhash[n];
> +
> +		while (s) {
> +			if (s->is_static)
> +				warn("\"%s\" [%s] is a static %s\n",
> +				     s->name, s->module->name,
> +				     export_str(s->export));
> +
> +			s = s->next;
> +		}
> +	}
> +
>   	free(buf.p);
>   
>   	return err;
>