LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3] module: Fix display of wrong  module .text address
@ 2018-04-18  7:14 Thomas Richter
  2018-04-18  7:17 ` Tobin C. Harding
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Thomas Richter @ 2018-04-18  7:14 UTC (permalink / raw)
  To: jeyu, torvalds, linux-kernel
  Cc: borntraeger, schwidefsky, brueckner, heiko.carstens, peterz,
	acme, me, keescook, stable, Thomas Richter

Reading file /proc/modules shows the correct address:
[root@s35lp76 ~]# cat /proc/modules | egrep '^qeth_l2'
qeth_l2 94208 1 - Live 0x000003ff80401000

and reading file /sys/module/qeth_l2/sections/.text
[root@s35lp76 ~]# cat /sys/module/qeth_l2/sections/.text
0x0000000018ea8363
displays a random address.

This breaks the perf tool which uses this address on s390
to calculate start of .text section in memory.

Fix this by printing the correct (unhashed) address.

Thanks to Jessica Yu for helping on this.

Fixes: ef0010a30935 ("vsprintf: don't use 'restricted_pointer()' when not restricting")
Cc: <stable@vger.kernel.org> # v4.15+
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Cc: Jessica Yu <jeyu@kernel.org>
---
 kernel/module.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index a6e43a5806a1..40b42000bd80 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1472,7 +1472,8 @@ 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);
+	return sprintf(buf, "0x%px\n", kptr_restrict < 2 ?
+		       (void *)sattr->address : NULL);
 }
 
 static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
-- 
2.14.3

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] module: Fix display of wrong  module .text address
  2018-04-18  7:14 [PATCH v3] module: Fix display of wrong module .text address Thomas Richter
@ 2018-04-18  7:17 ` Tobin C. Harding
  2018-04-18  7:22   ` Thomas-Mich Richter
  2018-04-18 13:21 ` Jessica Yu
  2018-05-02  2:20 ` Kees Cook
  2 siblings, 1 reply; 8+ messages in thread
From: Tobin C. Harding @ 2018-04-18  7:17 UTC (permalink / raw)
  To: Thomas Richter
  Cc: jeyu, torvalds, linux-kernel, borntraeger, schwidefsky,
	brueckner, heiko.carstens, peterz, acme, keescook, stable

On Wed, Apr 18, 2018 at 09:14:36AM +0200, Thomas Richter wrote:
> Reading file /proc/modules shows the correct address:
> [root@s35lp76 ~]# cat /proc/modules | egrep '^qeth_l2'
> qeth_l2 94208 1 - Live 0x000003ff80401000
> 
> and reading file /sys/module/qeth_l2/sections/.text
> [root@s35lp76 ~]# cat /sys/module/qeth_l2/sections/.text
> 0x0000000018ea8363
> displays a random address.
> 
> This breaks the perf tool which uses this address on s390
> to calculate start of .text section in memory.
> 
> Fix this by printing the correct (unhashed) address.
> 
> Thanks to Jessica Yu for helping on this.
> 
> Fixes: ef0010a30935 ("vsprintf: don't use 'restricted_pointer()' when not restricting")
> Cc: <stable@vger.kernel.org> # v4.15+
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
> Cc: Jessica Yu <jeyu@kernel.org>
> ---

What's changed in each version please?


thanks,
Tobin.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] module: Fix display of wrong module .text address
  2018-04-18  7:17 ` Tobin C. Harding
@ 2018-04-18  7:22   ` Thomas-Mich Richter
  0 siblings, 0 replies; 8+ messages in thread
From: Thomas-Mich Richter @ 2018-04-18  7:22 UTC (permalink / raw)
  To: Tobin C. Harding
  Cc: jeyu, torvalds, linux-kernel, borntraeger, schwidefsky,
	brueckner, heiko.carstens, peterz, acme, keescook, stable

On 04/18/2018 09:17 AM, Tobin C. Harding wrote:
> On Wed, Apr 18, 2018 at 09:14:36AM +0200, Thomas Richter wrote:
>> Reading file /proc/modules shows the correct address:
>> [root@s35lp76 ~]# cat /proc/modules | egrep '^qeth_l2'
>> qeth_l2 94208 1 - Live 0x000003ff80401000
>>
>> and reading file /sys/module/qeth_l2/sections/.text
>> [root@s35lp76 ~]# cat /sys/module/qeth_l2/sections/.text
>> 0x0000000018ea8363
>> displays a random address.
>>
>> This breaks the perf tool which uses this address on s390
>> to calculate start of .text section in memory.
>>
>> Fix this by printing the correct (unhashed) address.
>>
>> Thanks to Jessica Yu for helping on this.
>>
>> Fixes: ef0010a30935 ("vsprintf: don't use 'restricted_pointer()' when not restricting")
>> Cc: <stable@vger.kernel.org> # v4.15+
>> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
>> Cc: Jessica Yu <jeyu@kernel.org>
>> ---
> 
> What's changed in each version please?
> 
> 
> thanks,
> Tobin.
> 

V2: Changed sprintf format string from %#lx to 0x%px (suggested by Kees Cook).
V3: Changed sprintf agrument from 0 to NULL to avoid sparse warning.

-- 
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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] module: Fix display of wrong  module .text address
  2018-04-18  7:14 [PATCH v3] module: Fix display of wrong module .text address Thomas Richter
  2018-04-18  7:17 ` Tobin C. Harding
@ 2018-04-18 13:21 ` Jessica Yu
  2018-04-18 18:09   ` Linus Torvalds
  2018-05-02  2:20 ` Kees Cook
  2 siblings, 1 reply; 8+ messages in thread
From: Jessica Yu @ 2018-04-18 13:21 UTC (permalink / raw)
  To: Thomas Richter
  Cc: torvalds, linux-kernel, borntraeger, schwidefsky, brueckner,
	heiko.carstens, peterz, acme, me, keescook, stable

+++ Thomas Richter [18/04/18 09:14 +0200]:
>Reading file /proc/modules shows the correct address:
>[root@s35lp76 ~]# cat /proc/modules | egrep '^qeth_l2'
>qeth_l2 94208 1 - Live 0x000003ff80401000
>
>and reading file /sys/module/qeth_l2/sections/.text
>[root@s35lp76 ~]# cat /sys/module/qeth_l2/sections/.text
>0x0000000018ea8363
>displays a random address.
>
>This breaks the perf tool which uses this address on s390
>to calculate start of .text section in memory.
>
>Fix this by printing the correct (unhashed) address.
>
>Thanks to Jessica Yu for helping on this.
>
>Fixes: ef0010a30935 ("vsprintf: don't use 'restricted_pointer()' when not restricting")
>Cc: <stable@vger.kernel.org> # v4.15+
>Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
>Cc: Jessica Yu <jeyu@kernel.org>

Thanks for turning this into a real patch. Looks good to me, if Linus
wants to take it directly:

Acked-by: Jessica Yu <jeyu@kernel.org>

Otherwise I can take this up my tree.

As Linus mentioned, we should technically be checking the opening
task's credentials/capabilities, but converting everything to pass the
actual struct seq_file/file to the sysfs show handlers would be fairly
painful :/ At least those /sys/module/*/sections/ files are all set to
0400. I think this fix is sufficient for now.

>---
> kernel/module.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index a6e43a5806a1..40b42000bd80 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -1472,7 +1472,8 @@ 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);
>+	return sprintf(buf, "0x%px\n", kptr_restrict < 2 ?
>+		       (void *)sattr->address : NULL);
> }
>
> static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
>-- 
>2.14.3
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] module: Fix display of wrong module .text address
  2018-04-18 13:21 ` Jessica Yu
@ 2018-04-18 18:09   ` Linus Torvalds
  0 siblings, 0 replies; 8+ messages in thread
From: Linus Torvalds @ 2018-04-18 18:09 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Thomas Richter, Linux Kernel Mailing List, Christian Borntraeger,
	Martin Schwidefsky, Hendrik Brueckner, Heiko Carstens,
	Peter Zijlstra, Arnaldo Carvalho de Melo, tcharding, Kees Cook,
	stable

On Wed, Apr 18, 2018 at 6:21 AM, Jessica Yu <jeyu@kernel.org> wrote:
>
> Otherwise I can take this up my tree.

Take it through your tree.

It's not some recent thing that stops people from testing other stuff
(which is when I tend to try to take it directly just to short-circuit
any delays), so it might as well go through the "proper channels".

Thanks,

                Linus

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] module: Fix display of wrong module .text address
  2018-04-18  7:14 [PATCH v3] module: Fix display of wrong module .text address Thomas Richter
  2018-04-18  7:17 ` Tobin C. Harding
  2018-04-18 13:21 ` Jessica Yu
@ 2018-05-02  2:20 ` Kees Cook
  2018-05-02  8:13   ` Thomas-Mich Richter
  2 siblings, 1 reply; 8+ messages in thread
From: Kees Cook @ 2018-05-02  2:20 UTC (permalink / raw)
  To: Thomas Richter
  Cc: Jessica Yu, Linus Torvalds, LKML, Christian Borntraeger,
	Martin Schwidefsky, brueckner, Heiko Carstens, Peter Zijlstra,
	Arnaldo Carvalho de Melo, Tobin C. Harding, # 3.4.x

On Wed, Apr 18, 2018 at 12:14 AM, Thomas Richter <tmricht@linux.ibm.com> wrote:
> Reading file /proc/modules shows the correct address:
> [root@s35lp76 ~]# cat /proc/modules | egrep '^qeth_l2'
> qeth_l2 94208 1 - Live 0x000003ff80401000
>
> and reading file /sys/module/qeth_l2/sections/.text
> [root@s35lp76 ~]# cat /sys/module/qeth_l2/sections/.text
> 0x0000000018ea8363
> displays a random address.
>
> This breaks the perf tool which uses this address on s390
> to calculate start of .text section in memory.
>
> Fix this by printing the correct (unhashed) address.
>
> Thanks to Jessica Yu for helping on this.
>
> Fixes: ef0010a30935 ("vsprintf: don't use 'restricted_pointer()' when not restricting")
> Cc: <stable@vger.kernel.org> # v4.15+
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
> Cc: Jessica Yu <jeyu@kernel.org>
> ---
>  kernel/module.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index a6e43a5806a1..40b42000bd80 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1472,7 +1472,8 @@ 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);
> +       return sprintf(buf, "0x%px\n", kptr_restrict < 2 ?
> +                      (void *)sattr->address : NULL);

Errr... this looks reversed to me.

I would expect: "kptr_restrict < 2 ? NULL : (void *)sattr->address"

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] module: Fix display of wrong module .text address
  2018-05-02  2:20 ` Kees Cook
@ 2018-05-02  8:13   ` Thomas-Mich Richter
  2018-05-02 14:20     ` Kees Cook
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas-Mich Richter @ 2018-05-02  8:13 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jessica Yu, Linus Torvalds, LKML, Christian Borntraeger,
	Martin Schwidefsky, brueckner, Heiko Carstens, Peter Zijlstra,
	Arnaldo Carvalho de Melo, Tobin C. Harding, # 3.4.x

On 05/02/2018 04:20 AM, Kees Cook wrote:
> On Wed, Apr 18, 2018 at 12:14 AM, Thomas Richter <tmricht@linux.ibm.com> wrote:
>> Reading file /proc/modules shows the correct address:
>> [root@s35lp76 ~]# cat /proc/modules | egrep '^qeth_l2'
>> qeth_l2 94208 1 - Live 0x000003ff80401000
>>
>> and reading file /sys/module/qeth_l2/sections/.text
>> [root@s35lp76 ~]# cat /sys/module/qeth_l2/sections/.text
>> 0x0000000018ea8363
>> displays a random address.
>>
>> This breaks the perf tool which uses this address on s390
>> to calculate start of .text section in memory.
>>
>> Fix this by printing the correct (unhashed) address.
>>
>> Thanks to Jessica Yu for helping on this.
>>
>> Fixes: ef0010a30935 ("vsprintf: don't use 'restricted_pointer()' when not restricting")
>> Cc: <stable@vger.kernel.org> # v4.15+
>> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
>> Cc: Jessica Yu <jeyu@kernel.org>
>> ---
>>  kernel/module.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index a6e43a5806a1..40b42000bd80 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1472,7 +1472,8 @@ 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);
>> +       return sprintf(buf, "0x%px\n", kptr_restrict < 2 ?
>> +                      (void *)sattr->address : NULL);
> 
> Errr... this looks reversed to me.
> 
> I would expect: "kptr_restrict < 2 ? NULL : (void *)sattr->address"
> 
> -Kees
> 

I am confused:
In my patch, if kptr_restrict == 2 it prints NULL, which kptr_restrict 
being 0 or 1 it prints the address.

In your comment if kptr_restrict == 2 it prints the address, which 
kptr_restrict being 0 or 1 it prints NULL.

Looking into Documentation/sysctl/kernel.txt:
  When kptr_restrict is set to (2), kernel pointers printed using
  %pK will be replaced with 0's regardless of privileges.

With my patch, setting kptr_restrict to 0 or 1
prints the real kernel address (format %px, unmodified address
according to Documentation/printk-formats.txt).

I have tested this on s390 (which is the only arch using file
/sys/module/<XXX>/sections/.text) in the perf tool.

root@s8360047 ~]# sysctl  kernel.kptr_restrict
kernel.kptr_restrict = 0
[root@s8360047 ~]# cat /proc/modules | egrep '^qeth_l2'
qeth_l2 102400 1 - Live 0x000003ff8034d000
[root@s8360047 ~]# cat /sys/module/qeth_l2/sections/.text 
0x000003ff8034da68
[root@s8360047 ~]# sysctl  -w kernel.kptr_restrict=2
kernel.kptr_restrict = 2
[root@s8360047 ~]# cat /proc/modules | egrep '^qeth_l2'
qeth_l2 102400 1 - Live 0x0000000000000000
[root@s8360047 ~]# cat /sys/module/qeth_l2/sections/.text 
0x0000000000000000
[root@s8360047 ~]# uname -a
Linux s8360047 4.17.0-rc3m-perf+ #6 SMP PREEMPT Wed May 2 10:02:38 CEST 2018 s390x s390x s390x GNU/Linux
[root@s8360047 ~]# 

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] module: Fix display of wrong module .text address
  2018-05-02  8:13   ` Thomas-Mich Richter
@ 2018-05-02 14:20     ` Kees Cook
  0 siblings, 0 replies; 8+ messages in thread
From: Kees Cook @ 2018-05-02 14:20 UTC (permalink / raw)
  To: Thomas-Mich Richter
  Cc: Jessica Yu, Linus Torvalds, LKML, Christian Borntraeger,
	Martin Schwidefsky, brueckner, Heiko Carstens, Peter Zijlstra,
	Arnaldo Carvalho de Melo, Tobin C. Harding, # 3.4.x

On Wed, May 2, 2018 at 1:13 AM, Thomas-Mich Richter
<tmricht@linux.ibm.com> wrote:
> On 05/02/2018 04:20 AM, Kees Cook wrote:
>> On Wed, Apr 18, 2018 at 12:14 AM, Thomas Richter <tmricht@linux.ibm.com> wrote:
>>> Reading file /proc/modules shows the correct address:
>>> [root@s35lp76 ~]# cat /proc/modules | egrep '^qeth_l2'
>>> qeth_l2 94208 1 - Live 0x000003ff80401000
>>>
>>> and reading file /sys/module/qeth_l2/sections/.text
>>> [root@s35lp76 ~]# cat /sys/module/qeth_l2/sections/.text
>>> 0x0000000018ea8363
>>> displays a random address.
>>>
>>> This breaks the perf tool which uses this address on s390
>>> to calculate start of .text section in memory.
>>>
>>> Fix this by printing the correct (unhashed) address.
>>>
>>> Thanks to Jessica Yu for helping on this.
>>>
>>> Fixes: ef0010a30935 ("vsprintf: don't use 'restricted_pointer()' when not restricting")
>>> Cc: <stable@vger.kernel.org> # v4.15+
>>> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>>> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
>>> Cc: Jessica Yu <jeyu@kernel.org>
>>> ---
>>>  kernel/module.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/module.c b/kernel/module.c
>>> index a6e43a5806a1..40b42000bd80 100644
>>> --- a/kernel/module.c
>>> +++ b/kernel/module.c
>>> @@ -1472,7 +1472,8 @@ 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);
>>> +       return sprintf(buf, "0x%px\n", kptr_restrict < 2 ?
>>> +                      (void *)sattr->address : NULL);
>>
>> Errr... this looks reversed to me.
>>
>> I would expect: "kptr_restrict < 2 ? NULL : (void *)sattr->address"
>>
>> -Kees
>>
>
> I am confused:
> In my patch, if kptr_restrict == 2 it prints NULL, which kptr_restrict
> being 0 or 1 it prints the address.
>
> In your comment if kptr_restrict == 2 it prints the address, which
> kptr_restrict being 0 or 1 it prints NULL.
>
> Looking into Documentation/sysctl/kernel.txt:
>   When kptr_restrict is set to (2), kernel pointers printed using
>   %pK will be replaced with 0's regardless of privileges.
>
> With my patch, setting kptr_restrict to 0 or 1
> prints the real kernel address (format %px, unmodified address
> according to Documentation/printk-formats.txt).
>
> I have tested this on s390 (which is the only arch using file
> /sys/module/<XXX>/sections/.text) in the perf tool.
>
> root@s8360047 ~]# sysctl  kernel.kptr_restrict
> kernel.kptr_restrict = 0
> [root@s8360047 ~]# cat /proc/modules | egrep '^qeth_l2'
> qeth_l2 102400 1 - Live 0x000003ff8034d000
> [root@s8360047 ~]# cat /sys/module/qeth_l2/sections/.text
> 0x000003ff8034da68
> [root@s8360047 ~]# sysctl  -w kernel.kptr_restrict=2
> kernel.kptr_restrict = 2
> [root@s8360047 ~]# cat /proc/modules | egrep '^qeth_l2'
> qeth_l2 102400 1 - Live 0x0000000000000000
> [root@s8360047 ~]# cat /sys/module/qeth_l2/sections/.text
> 0x0000000000000000
> [root@s8360047 ~]# uname -a
> Linux s8360047 4.17.0-rc3m-perf+ #6 SMP PREEMPT Wed May 2 10:02:38 CEST 2018 s390x s390x s390x GNU/Linux
> [root@s8360047 ~]#
>
> Hope this helps.

Thanks! Yes, I was looking at too many of the %px commits in a row and
confused myself. Sorry for the noise!

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-05-02 14:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-18  7:14 [PATCH v3] module: Fix display of wrong module .text address Thomas Richter
2018-04-18  7:17 ` Tobin C. Harding
2018-04-18  7:22   ` Thomas-Mich Richter
2018-04-18 13:21 ` Jessica Yu
2018-04-18 18:09   ` Linus Torvalds
2018-05-02  2:20 ` Kees Cook
2018-05-02  8:13   ` Thomas-Mich Richter
2018-05-02 14:20     ` Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).