Re: [PATCH net-next v3] net: Replace %pK output with 0

Hi Sebastian,

On 14.08.26 03:58, Jakub Kicinski wrote:

quoted

diff --git a/net/can/proc.c b/net/can/proc.c
index de4d05ae3459..4314180fd7a1 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c

@@ -192,12 +192,11 @@ static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list,
  
  	hlist_for_each_entry_rcu(r, rx_list, list) {
  		char *fmt = (r->can_id & CAN_EFF_FLAG)?
-			"   %-5s  %08x  %08x  %pK  %pK  %8ld  %s\n" :
-			"   %-5s     %03x    %08x  %pK  %pK  %8ld  %s\n";
+			"   %-5s  %08x  %08x  %-20ps %8u %8ld  %s\n" :
+			"   %-5s     %03x    %08x  %-20ps %8u %8ld  %s\n";
  
  		seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
-			   r->func, r->data, atomic_long_read(&r->matches),
-			   r->ident);
+			   r->func, 0, atomic_long_read(&r->matches), r->ident);
  	}
  }

[Severity: Medium]
Does this widen what unprivileged readers can see, rather than narrow it?
The rcvlist_* files are created world readable by can_init_proc():

   net/can/proc.c:can_init_proc() {
       ...
       net->can.pde_rcvlist_all = proc_create_net_single(CAN_PROC_RCVLIST_ALL,
                       0644, net->can.proc_dir, can_rcvlist_proc_show,
                       (void *)RX_ALL);
       ...
   }

Unlike %pK, %ps is not filtered by kptr_restrict and is never hashed.  With
CONFIG_KALLSYMS=n, symbol_string() takes the fallback branch:

I got the point but the patch rendered the CAN proc outputs partly 
useless and it is definitely time to give it a rework.

It is pretty cool following your suggestion to be able to see the 
function calls like "bcm_rx_handler [can_bcm]". This is always what I 
wanted and brings no new security nor privacy issues. It mainly makes 
the ident string obsolete.

Instead of the other "data" it makes sense to provide the sock inode 
number to identify the relevant socket where possible (for raw/bcm/isotp).

If its ok for you I would provide a separate patch for can-next 
reworking the CAN proc output?!? So you would be able to continue with 
the other hunks only.

I'll put you and netdev ML in CC.

Best regards,
Oliver

   lib/vsprintf.c:symbol_string() {
       ...
       return string_nocheck(buf, end, sym, spec);
   #else
       return special_hex_number(buf, end, value, sizeof(void *));
   #endif
   }

so the raw kernel/module text address of the receive callback is printed to
any local reader, even with kptr_restrict=2.

With CONFIG_KALLSYMS=y the address is not printed, but the symbol plus the
owning module name is now disclosed unconditionally, since
__sprint_symbol() appends the module:

   kernel/kallsyms.c:__sprint_symbol() {
       ...
       if (modname) {
               len += sprintf(buffer + len, " [%s", modname);
       ...
   }

Previously kptr_restrict >= 1 censored this field for readers without
CAP_SYSLOG.  Would the literal 0 placeholder used for the other 14 sites
work here too?

quoted

@@ -207,10 +206,7 @@ static void can_print_recv_banner(struct seq_file *m)
  	 *                  can1.  00000000  00000000  00000000
  	 *                 .......          0  tp20
  	 */

[Severity: Low]
This example comment still shows the old layout, where the receive callback
was an 8-hex-digit %pK value (the third 00000000 column) and userdata was a
pointer.  can_print_rcvlist() now emits a left-justified symbol name via
%-20ps and a constant 0 for userdata, so nothing the code can produce
matches this example.  Should the comment be refreshed?

quoted

-	if (IS_ENABLED(CONFIG_64BIT))
-		seq_puts(m, "  device   can_id   can_mask      function          userdata       matches  ident\n");
-	else
-		seq_puts(m, "  device   can_id   can_mask  function  userdata   matches  ident\n");
+	seq_puts(m, "  device   can_id   can_mask  function             userdata  matches  ident\n");
  }

[Severity: Low]
The new banner advertises a fixed 20-character "function" column, but does
the row format actually stay inside it?

Module symbols printed by %ps carry the module suffix, e.g.
"bcm_rx_handler [can_bcm]" (24 chars), "j1939_can_recv [can_j1939]" (26)
or "isotp_rcv [can_isotp]" (21), so userdata/matches/ident get pushed to
the right whenever can_bcm, can_isotp or can_j1939 is loaded.

In the CONFIG_KALLSYMS=n case symbol_string() calls

       return special_hex_number(buf, end, value, sizeof(void *));

which discards the caller's spec, so the field is an unpadded 18-character
0xffffffffXXXXXXXX and the following columns shift left instead.

[Severity: Low]
Documentation/networking/can.rst still documents the previous output of
/proc/net/can/rcvlist_*:

         device   can_id   can_mask  function  userdata   matches  ident
          vcan0     000    00000000  f88e6370  f6c6f400         0  raw

After this change the banner is spaced differently, "function" is a symbol
name that may itself contain a space before "[module]", and "userdata" is
always 0.  Should can.rst be updated in the same patch?

quoted

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index b8887cdd66c5..2cd8ae4a0e4b 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c

[ ... ]

quoted

@@ -2811,7 +2810,7 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i)
  					      READ_ONCE(tp->copied_seq), 0);
  
  	seq_printf(f, "%4d: %08X:%04X %08X:%04X %02X %08X:%08X %02X:%08lX "
-			"%08X %5u %8d %llu %d %pK %lu %lu %u %u %d",
+			"%08X %5u %8d %llu %d 0 %lu %lu %u %u %d",

[Severity: Low]
Documentation/networking/proc_net_tcp.rst still describes this field as the

     |          |    |     |    |------------------> location of socket in memory

and names tcp4_seq_show()/tcp6_seq_show() as the implementation, with a
nonzero example value.  With get_tcp4_sock(), get_tcp6_sock(),
get_openreq4()/get_openreq6() and get_timewait4_sock()/get_timewait6_sock()
all emitting a literal 0 there, no code path can produce the documented
value any more.  Could that documentation be adjusted along with this
change?

[ ... ]