Wrong module .text address in 4.16.0

Wrong module .text address in 4.16.0

[Thomas-Mich Richter]

I just installed 4.16.0 and discovered the module .text address is
wrong. It happens on s390 and x86 platforms. I have not tested others.

Here is the issue, I have used module qeth_l2 on s390 which is the
ethernet device driver:

root@s35lp76 ~]# lsmod
Module                  Size  Used by
qeth_l2                94208  1
...

[root@s35lp76 ~]# cat /proc/modules | egrep '^qeth_l2'
qeth_l2 94208 1 - Live 0x000003ff80401000   <---- This is the correct address in memory
[root@s35lp76 ~]# cat /sys/module/qeth_l2/sections/.text 
0x0000000018ea8363      <---- This is the wrong address
[root@s35lp76 ~]# 

File /sys/module/qeth_l2/sections/.text displays a very strange
address which is definitely wrong. It should be something like
0x000003ff80401xxx.

Same on x86.

I have checked file kernel/module.c function add_sect_attrs()
and it calls module_sect_show() when the sysfs file is read.
And module_sect_show() uses 

  sprintf(buf, "0x%pK\n", (void *)sattr->address);

and my sysctl setting should be correct:
[root@s35lp76 linux]# sysctl -a | fgrep kernel.kptr_restrict
kernel.kptr_restrict = 0
[root@s35lp76 linux]#

I wonder if somebody else has seen this issue?
Ideas how to fix this?

Thanks
-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: Wrong module .text address in 4.16.0

[Christian Borntraeger]

FWIW, this breaks at least perf capability to resolve module symbols.
Adding some more CCs for perf and module.


On 04/16/2018 07:51 AM, Thomas-Mich Richter wrote:
> I just installed 4.16.0 and discovered the module .text address is
> wrong. It happens on s390 and x86 platforms. I have not tested others.
> 
> Here is the issue, I have used module qeth_l2 on s390 which is the
> ethernet device driver:
> 
> root@s35lp76 ~]# lsmod
> Module                  Size  Used by
> qeth_l2                94208  1
> ...
> 
> [root@s35lp76 ~]# cat /proc/modules | egrep '^qeth_l2'
> qeth_l2 94208 1 - Live 0x000003ff80401000   <---- This is the correct address in memory
> [root@s35lp76 ~]# cat /sys/module/qeth_l2/sections/.text 
> 0x0000000018ea8363      <---- This is the wrong address
> [root@s35lp76 ~]# 
> 
> File /sys/module/qeth_l2/sections/.text displays a very strange
> address which is definitely wrong. It should be something like
> 0x000003ff80401xxx.
> 
> Same on x86.
> 
> I have checked file kernel/module.c function add_sect_attrs()
> and it calls module_sect_show() when the sysfs file is read.
> And module_sect_show() uses 
> 
>   sprintf(buf, "0x%pK\n", (void *)sattr->address);
> 
> and my sysctl setting should be correct:
> [root@s35lp76 linux]# sysctl -a | fgrep kernel.kptr_restrict
> kernel.kptr_restrict = 0
> [root@s35lp76 linux]#
> 
> I wonder if somebody else has seen this issue?
> Ideas how to fix this?
> 
> Thanks
> 

Re: Wrong module .text address in 4.16.0

[Christian Borntraeger]

Can this be related to 
commit ef0010a30935de4e0211cbc7bdffc30446cdee9b
    vsprintf: don't use 'restricted_pointer()' when not restricting
and related commits?


To me it looks like %pk is always printing the hash, but never the real pointer - 
no matter what kernel.kptr_restrict says.



On 04/16/2018 08:23 AM, Christian Borntraeger wrote:
> FWIW, this breaks at least perf capability to resolve module symbols.
> Adding some more CCs for perf and module.
> 
> 
> On 04/16/2018 07:51 AM, Thomas-Mich Richter wrote:
>> I just installed 4.16.0 and discovered the module .text address is
>> wrong. It happens on s390 and x86 platforms. I have not tested others.
>>
>> Here is the issue, I have used module qeth_l2 on s390 which is the
>> ethernet device driver:
>>
>> root@s35lp76 ~]# lsmod
>> Module                  Size  Used by
>> qeth_l2                94208  1
>> ...
>>
>> [root@s35lp76 ~]# cat /proc/modules | egrep '^qeth_l2'
>> qeth_l2 94208 1 - Live 0x000003ff80401000   <---- This is the correct address in memory
>> [root@s35lp76 ~]# cat /sys/module/qeth_l2/sections/.text 
>> 0x0000000018ea8363      <---- This is the wrong address
>> [root@s35lp76 ~]# 
>>
>> File /sys/module/qeth_l2/sections/.text displays a very strange
>> address which is definitely wrong. It should be something like
>> 0x000003ff80401xxx.
>>
>> Same on x86.
>>
>> I have checked file kernel/module.c function add_sect_attrs()
>> and it calls module_sect_show() when the sysfs file is read.
>> And module_sect_show() uses 
>>
>>   sprintf(buf, "0x%pK\n", (void *)sattr->address);
>>
>> and my sysctl setting should be correct:
>> [root@s35lp76 linux]# sysctl -a | fgrep kernel.kptr_restrict
>> kernel.kptr_restrict = 0
>> [root@s35lp76 linux]#
>>
>> I wonder if somebody else has seen this issue?
>> Ideas how to fix this?
>>
>> Thanks
>>

Re: Wrong module .text address in 4.16.0

[Thomas-Mich Richter]

On 04/16/2018 08:23 AM, Christian Borntraeger wrote:
> FWIW, this breaks at least perf capability to resolve module symbols.
> Adding some more CCs for perf and module.
> 
> 
> On 04/16/2018 07:51 AM, Thomas-Mich Richter wrote:
>> I just installed 4.16.0 and discovered the module .text address is
>> wrong. It happens on s390 and x86 platforms. I have not tested others.
>>
>> Here is the issue, I have used module qeth_l2 on s390 which is the
>> ethernet device driver:
>>
>> root@s35lp76 ~]# lsmod
>> Module                  Size  Used by
>> qeth_l2                94208  1
>> ...
>>
>> [root@s35lp76 ~]# cat /proc/modules | egrep '^qeth_l2'
>> qeth_l2 94208 1 - Live 0x000003ff80401000   <---- This is the correct address in memory
>> [root@s35lp76 ~]# cat /sys/module/qeth_l2/sections/.text 
>> 0x0000000018ea8363      <---- This is the wrong address
>> [root@s35lp76 ~]# 
>>
>> File /sys/module/qeth_l2/sections/.text displays a very strange
>> address which is definitely wrong. It should be something like
>> 0x000003ff80401xxx.
>>
>> Same on x86.
>>
>> I have checked file kernel/module.c function add_sect_attrs()
>> and it calls module_sect_show() when the sysfs file is read.
>> And module_sect_show() uses 
>>
>>   sprintf(buf, "0x%pK\n", (void *)sattr->address);
>>
>> and my sysctl setting should be correct:
>> [root@s35lp76 linux]# sysctl -a | fgrep kernel.kptr_restrict
>> kernel.kptr_restrict = 0
>> [root@s35lp76 linux]#
>>
>> I wonder if somebody else has seen this issue?
>> Ideas how to fix this?
>>
>> Thanks
>>

This new behavior actually break perf report/record/top on s390 for
module address resolution. On s390 each module .text segment actually
start at some offset after the module load addess shown with
/proc/modules. That is the reason why we need 
/sys/module/<module-name>/sections/.text to get the actual
.text segment start address.

-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: Wrong module .text address in 4.16.0

[Jessica Yu]

+++ Christian Borntraeger [16/04/18 12:53 +0200]:
>Can this be related to
>commit ef0010a30935de4e0211cbc7bdffc30446cdee9b
>    vsprintf: don't use 'restricted_pointer()' when not restricting
>and related commits?
>
>To me it looks like %pk is always printing the hash, but never the real pointer -
>no matter what kernel.kptr_restrict says.

(Added Kees and Tobin to CC)

Can confirm, a git bisect confirms that ef0010a3093 was the first bad commit.

And yeah, the default seems to be to always hash the pointer address now,
regardless of kptr_restrict. See Documentation/sysctl/kernel.txt:

    When kptr_restrict is set to 0 (the default) the address is hashed before
    printing. (This is the equivalent to %p.)

And to quote from the relevant patchset (https://lkml.org/lkml/2017/11/28/1593):

   The added advantage of hashing %p is that security is now opt-out, if
   you _really_ want the address you have to work a little harder and use %px.

So for users of /sys/module/*/sections, we will need to work around
this and possibly use %px for the real address. But perhaps we should
base the usage of %px on kptr_restrict? That is what m_show() in
module.c currently does, which is why you get the correct address when
you look at /proc/modules (it uses %px and either shows 0's or the
real address based on kallsyms_show_value()). module_sect_show() uses
%pK so it's getting hashed. Is there a better way of doing this?

Jessica

>
>
>On 04/16/2018 08:23 AM, Christian Borntraeger wrote:
>> FWIW, this breaks at least perf capability to resolve module symbols.
>> Adding some more CCs for perf and module.
>>
>>
>> On 04/16/2018 07:51 AM, Thomas-Mich Richter wrote:
>>> I just installed 4.16.0 and discovered the module .text address is
>>> wrong. It happens on s390 and x86 platforms. I have not tested others.
>>>
>>> Here is the issue, I have used module qeth_l2 on s390 which is the
>>> ethernet device driver:
>>>
>>> root@s35lp76 ~]# lsmod
>>> Module                  Size  Used by
>>> qeth_l2                94208  1
>>> ...
>>>
>>> [root@s35lp76 ~]# cat /proc/modules | egrep '^qeth_l2'
>>> qeth_l2 94208 1 - Live 0x000003ff80401000   <---- This is the correct address in memory
>>> [root@s35lp76 ~]# cat /sys/module/qeth_l2/sections/.text
>>> 0x0000000018ea8363      <---- This is the wrong address
>>> [root@s35lp76 ~]#
>>>
>>> File /sys/module/qeth_l2/sections/.text displays a very strange
>>> address which is definitely wrong. It should be something like
>>> 0x000003ff80401xxx.
>>>
>>> Same on x86.
>>>
>>> I have checked file kernel/module.c function add_sect_attrs()
>>> and it calls module_sect_show() when the sysfs file is read.
>>> And module_sect_show() uses
>>>
>>>   sprintf(buf, "0x%pK\n", (void *)sattr->address);
>>>
>>> and my sysctl setting should be correct:
>>> [root@s35lp76 linux]# sysctl -a | fgrep kernel.kptr_restrict
>>> kernel.kptr_restrict = 0
>>> [root@s35lp76 linux]#
>>>
>>> I wonder if somebody else has seen this issue?
>>> Ideas how to fix this?
>>>
>>> Thanks
>>>
>

Re: Wrong module .text address in 4.16.0

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]

On Mon, Apr 16, 2018 at 6:43 AM, Jessica Yu <jeyu@kernel.org> wrote:
>
> So for users of /sys/module/*/sections, we will need to work around
> this and possibly use %px for the real address. But perhaps we should
> base the usage of %px on kptr_restrict?

Maybe. I was hoping we would be able to get rid of it eventually.

The real problem is that those darn module_attribute things don't have
proper IO routines. They *only* have the show routine, and that
doesn't even get the 'struct file' pointer passed to it, just the
buffer to fill in (not even a _size_ of a buffer - we're talking the
bad bad old days of nasty /proc interfaces).

Why is that a problem? Without a 'struct file' we can't even do
permission checking right. %pK worked by doing disgusting wrong
things.

Now, in this case, at least the files are root-owned, and legible only
to root, so I guess we can say that permissions have been properly
checked at open time (not really true: the CAP_SYSLOG bit wasn't!, but
I doubt anybody really cares), and so we could just check
kptr_restrict.

Oh well.

Something like the attached, perhaps? Completely untested, and I don't
even want credit for this if it is used.

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 675 bytes --]

 kernel/module.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index a6e43a5806a1..f8cf0bb35ab6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1472,7 +1472,11 @@ static ssize_t module_sect_show(struct module_attribute *mattr,
 {
 	struct module_sect_attr *sattr =
 		container_of(mattr, struct module_sect_attr, mattr);
-	return sprintf(buf, "0x%pK\n", (void *)sattr->address);
+	unsigned long addr = 0;
+
+	/* Permissions were checked at open */
+	addr = kptr_restrict < 2 ?sattr->address : 0;
+	return sprintf(buf, "%#lx\n", addr);
 }
 
 static void free_sect_attrs(struct module_sect_attrs *sect_attrs)