linux-kernel - Re: [PATCH] modules: sysfs - export: taint, address, size

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: <CAPXgP12rUUYfRy6QB5Z4Rwdday3hNHubCsSRY4gMDYuQGs3oJg@mail.gmail.com>
Date:	Mon, 9 Jan 2012 13:44:52 +0100
From:	Kay Sievers <kay.sievers@...y.org>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	Jon Masters <jcm@...hat.com>, linux-kernel@...r.kernel.org,
	Lucas De Marchi <lucas.demarchi@...fusion.mobi>
Subject: Re: [PATCH] modules: sysfs - export: taint, address, size

On Mon, Jan 9, 2012 at 08:27, Rusty Russell <rusty@...tcorp.com.au> wrote:
> On Sat, 07 Jan 2012 16:44:36 +0100, Kay Sievers <kay.sievers@...y.org> wrote:

>> Recent tools do not use /proc to retrieve module information. A few values
>> are currently missing from sysfs.
>
> Well, strace says lsmod still does.  Is libkmod doing something
> different?

Yes, kmod used /sys only.

There is current code to read the size, to provide the 'lsmod' output,
but that will be removed.

> Should we be deprecating /proc/modules?

In the longer run, yes.

We still aim for leaving everything that isn't process- or
namespace-related (which, with some stretch is always process-related)
alone, and use /sys for it.

>> +static size_t module_flags_taint(struct module *mod, char *buf)
>> +{
>> +     size_t l = 0;
>> +
>> +     if (mod->taints & (1 << TAINT_PROPRIETARY_MODULE))
>> +             buf[l++] = 'P';
>> +     else if (mod->taints & (1 << TAINT_OOT_MODULE))
>> +             buf[l++] = 'O';
>> +     if (mod->taints & (1 << TAINT_FORCED_MODULE))
>> +             buf[l++] = 'F';
>> +     if (mod->taints & (1 << TAINT_CRAP))
>> +             buf[l++] = 'C';
>> +     /*
>> +      * TAINT_FORCED_RMMOD: could be added.
>> +      * TAINT_UNSAFE_SMP, TAINT_MACHINE_CHECK, TAINT_BAD_PAGE don't
>> +      * apply to modules.
>> +      */
>> +     return l;
>> +}
>
> The else here is weird.  Shouldn't we leave the exclusion elsewhere?

You mean the 'else if ... TAINT_OOT_MODULE'?  It's a one-to-one copy
of the current code, which just moved up a bit.

Disconnect the two flags form each other?

>> +static ssize_t show_address(struct module_attribute *mattr,
>> +                         struct module_kobject *mk, char *buffer)
>> +{
>> +     return sprintf(buffer, "0x%pK\n", mk->mod->module_core);
>> +}
>> +
>> +struct module_attribute module_address =
>> +     __ATTR(address, 0444, show_address, NULL);
>> +
>> +static ssize_t show_size(struct module_attribute *mattr,
>> +                     struct module_kobject *mk, char *buffer)
>> +{
>> +     return sprintf(buffer, "%u\n", mk->mod->init_size + mk->mod->core_size);
>> +}
>> +
>> +struct module_attribute module_size =
>> +     __ATTR(size, 0444, show_size, NULL);
>
> This copies a past mistake, and is definitely wrong.  Either expose both
> pointers and sizes, or don't include init_size here.  Sure, it'll
> normally be 0, but if not it's confusing...

Ah, good to know, mod->init_size is 0 for all modules here, so we
should just drop mod->init_size and maybe name the 'size' attribute to
'coresize'?

> But the bigger question is: Why are we exposing these sizes?
> /proc/modules did since 2.2, or before, but that doesn't make it the
> best option...

Good question, I doubt it is too useful, it's just that 'lsmod' shows
it, so we wanted to show too.

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/