Re: [PATCH net-next 05/37] can: proc: remove pointers from CAN specific proc output

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 10.10.26 16:02, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The commit says it stops /proc/net/can/ from leaking kernel
>    pointers, but on some builds it now leaks a raw kernel text address.
> - [Low] The procfs example in Documentation/networking/can.rst was
>    replaced, but the text around it was not updated.
> 
> --
> 
> can: proc: remove pointers from CAN specific proc output
> 
> This patch stops printing kernel pointers in /proc/net/can/rcvlist_* and
> /proc/net/can-bcm/. It prints socket inode numbers and %ps callback names
> instead. The ident string argument of can_rx_register() becomes a u64
> inode number, and the bcm proc file name is now the inode number in hex.
> 
> Link: https://patch.msgid.link/20260815103400.117175-1-socketcan@hartkopp.net
> 
>> diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst
>> index 536ff411da1d1..7bdb27a22a0ea 100644
>> --- a/Documentation/networking/can.rst
>> +++ b/Documentation/networking/can.rst
>> @@ -1042,15 +1042,15 @@ receive lists, their filters and the count of filter matches can be
>>   checked in the appropriate receive list. All entries contain the
>>   device and a protocol module identifier::
>>   
>> -    foo@bar:~$ cat /proc/net/can/rcvlist_all
>> +    foo@bar:~$ cat /proc/net/can/rcvlist_fil
>>   
>> -    receive list 'rx_all':
>> -      (vcan3: no entry)
>> -      (vcan2: no entry)
>> -      (vcan1: no entry)
>> -      device   can_id   can_mask  function  userdata   matches  ident
>> -       vcan0     000    00000000  f88e6370  f6c6f400         0  raw
>> +    receive list 'rx_fil':
>>         (any: no entry)
>> +      device   can_id   can_mask   matches     sock_inode     function
>> +       vcan0  80000123  c00007ff         0  000000000000f862  raw_rcv [can_raw]
>> +      (vcan1: no entry)
>> +      (vcan2: no entry)
>> +      (vcan3: no entry)
>>   
>>   In this example an application requests any CAN traffic from vcan0::
> 
> [Severity: Low]
> This isn't a bug, but the text around the new example still describes the
> old one. The sentence after the example says:
> 
>    In this example an application requests any CAN traffic from vcan0::
> 
> That matched the old rx_all entry, which had can_id 000 and mask 00000000.
> The new entry is in rx_fil with can_id 80000123 and can_mask c00007ff. It
> only matches non-RTR EFF frames whose low 11 ID bits are 0x123.
> 
> The sentence before the example also still says every entry contains "a
> protocol module identifier". The new function column is the %ps name of
> the callback. It only shows a "[can_raw]" style tag when the protocol is
> built as a module, and it is a plain number on !KALLSYMS builds.
> 
> Should this text be updated to match the new output?
> 
> [ ... ]
> 
>> diff --git a/net/can/proc.c b/net/can/proc.c
>> index 64b3bdc2fa7e0..33d99543e3fe2 100644
>> --- a/net/can/proc.c
>> +++ b/net/can/proc.c
>> @@ -215,25 +215,21 @@ 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";
>> +			"  %6s  %08x  %08x  %8ld  %016llx  %ps\n" :
>> +			"  %6s     %03x    %08x  %8ld  %016llx  %ps\n";
>>   
>>   		seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
>> -			   r->func, r->data, atomic_long_read(&r->matches),
>> -			   r->ident);
>> +			   atomic_long_read(&r->matches), r->ino, r->func);
>>   	}
>>   }
> 
> [Severity: High]
> On CONFIG_KALLSYMS=n builds, does can_print_rcvlist() now print a raw
> kernel text address?
> 
> The old format printed r->func with %pK. %pK follows kptr_restrict, so
> unprivileged readers saw a hashed or zeroed value. With %ps, symbol_string()
> falls back to the raw value when kallsyms is not built in:
> 
> lib/vsprintf.c:symbol_string() {
>      ...
> #else
> 	return special_hex_number(buf, end, value, sizeof(void *));
> #endif
> }
> 
> In that case the plain address of raw_rcv, bcm_rx_handler, isotp_rcv,
> isotp_rcv_echo, can_can_gw_rcv or j1939_can_recv would be printed.
> 
> can_init_proc() creates the rcvlist_* files with mode 0644. An unprivileged
> user can also add an entry by opening a CAN_RAW socket, through
> raw_enable_filters()->can_rx_register(). CAN has no KALLSYMS dependency in
> net/can/Kconfig, and KALLSYMS can be turned off under EXPERT.
> 
> Those builds also lose the old r->ident string ("raw", "bcm", "isotp",
> "gw", "j1939"). The column then shows only a number, not a protocol
> identifier.
> 
> Would a fixed per-protocol name work better here? Another option is to
> print a placeholder, or keep %pK, when CONFIG_KALLSYMS is off.
> 
> [ ... ]
> 

Addressed all sashiko-bot findings in a v2 patch here:

https://lore.kernel.org/linux-can/20261012115851.3412-1-socketcan@hartkopp.net/

Best regards,
Oliver