* [PATCH] CAN: Use inode instead of kernel address for /proc file
@ 2010-12-25 22:16 Dan Rosenberg
2010-12-25 22:22 ` Oliver Hartkopp
0 siblings, 1 reply; 6+ messages in thread
From: Dan Rosenberg @ 2010-12-25 22:16 UTC (permalink / raw)
To: Oliver Hartkopp, Urs Thuermann, David S. Miller; +Cc: netdev, security
Since the socket address is just being used as a unique identifier, its
inode number is an alternative that does not leak potentially sensitive
information.
CC-ing stable because MITRE has assigned CVE-2010-4565 to the issue.
Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
Cc: stable <stable@kernel.org>
---
net/can/bcm.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 6faa825..5748901 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1520,8 +1520,8 @@ static int bcm_connect(struct socket *sock, struct sockaddr *uaddr, int len,
bo->bound = 1;
if (proc_dir) {
- /* unique socket address as filename */
- sprintf(bo->procname, "%p", sock);
+ /* socket inode as filename */
+ sprintf(bo->procname, "%lx", sock_i_ino(sk));
bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
proc_dir,
&bcm_proc_fops, sk);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] CAN: Use inode instead of kernel address for /proc file
2010-12-25 22:16 [PATCH] CAN: Use inode instead of kernel address for /proc file Dan Rosenberg
@ 2010-12-25 22:22 ` Oliver Hartkopp
2010-12-25 22:31 ` Oliver Hartkopp
0 siblings, 1 reply; 6+ messages in thread
From: Oliver Hartkopp @ 2010-12-25 22:22 UTC (permalink / raw)
To: Dan Rosenberg; +Cc: Urs Thuermann, David S. Miller, netdev, security
On 25.12.2010 23:16, Dan Rosenberg wrote:
> Since the socket address is just being used as a unique identifier, its
> inode number is an alternative that does not leak potentially sensitive
> information.
>
> CC-ing stable because MITRE has assigned CVE-2010-4565 to the issue.
>
> Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> Cc: stable <stable@kernel.org>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Thanks Dan.
> ---
> net/can/bcm.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 6faa825..5748901 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -1520,8 +1520,8 @@ static int bcm_connect(struct socket *sock, struct sockaddr *uaddr, int len,
> bo->bound = 1;
>
> if (proc_dir) {
> - /* unique socket address as filename */
> - sprintf(bo->procname, "%p", sock);
> + /* socket inode as filename */
> + sprintf(bo->procname, "%lx", sock_i_ino(sk));
> bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
> proc_dir,
> &bcm_proc_fops, sk);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CAN: Use inode instead of kernel address for /proc file
2010-12-25 22:22 ` Oliver Hartkopp
@ 2010-12-25 22:31 ` Oliver Hartkopp
2010-12-25 22:41 ` Dan Rosenberg
0 siblings, 1 reply; 6+ messages in thread
From: Oliver Hartkopp @ 2010-12-25 22:31 UTC (permalink / raw)
To: Dan Rosenberg; +Cc: Urs Thuermann, David S. Miller, netdev, security
On 25.12.2010 23:22, Oliver Hartkopp wrote:
> On 25.12.2010 23:16, Dan Rosenberg wrote:
>> Since the socket address is just being used as a unique identifier, its
>> inode number is an alternative that does not leak potentially sensitive
>> information.
>>
>> CC-ing stable because MITRE has assigned CVE-2010-4565 to the issue.
>>
>> Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
>> Cc: stable <stable@kernel.org>
>
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
>
> Thanks Dan.
>
>> ---
>> net/can/bcm.c | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>> index 6faa825..5748901 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
>> @@ -1520,8 +1520,8 @@ static int bcm_connect(struct socket *sock, struct sockaddr *uaddr, int len,
>> bo->bound = 1;
>>
>> if (proc_dir) {
>> - /* unique socket address as filename */
>> - sprintf(bo->procname, "%p", sock);
>> + /* socket inode as filename */
>> + sprintf(bo->procname, "%lx", sock_i_ino(sk));
One minor question:
AFAIK the inode numbers that can be found in /proc/<pid>/fd/* are in decimal
and not in hex, right?
If so, you should use '%lu' instead of '%lx' in the patch.
Regards,
Oliver
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CAN: Use inode instead of kernel address for /proc file
2010-12-25 22:31 ` Oliver Hartkopp
@ 2010-12-25 22:41 ` Dan Rosenberg
2010-12-26 11:29 ` Oliver Hartkopp
0 siblings, 1 reply; 6+ messages in thread
From: Dan Rosenberg @ 2010-12-25 22:41 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Urs Thuermann, David S. Miller, netdev, security
>
> One minor question:
>
> AFAIK the inode numbers that can be found in /proc/<pid>/fd/* are in decimal
> and not in hex, right?
>
> If so, you should use '%lu' instead of '%lx' in the patch.
Yes, that's usually how they're expressed, but I did it this way for two
reasons. Firstly, %lu would require another change to the buffer size,
since the output could be up to 20 bytes long (plus another for the NULL
terminator). Secondly, by expressing it as hex it avoids breaking any
userland utilities that are expecting addresses, even if no such
utilities exist.
-Dan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CAN: Use inode instead of kernel address for /proc file
2010-12-25 22:41 ` Dan Rosenberg
@ 2010-12-26 11:29 ` Oliver Hartkopp
2010-12-26 16:38 ` Dan Rosenberg
0 siblings, 1 reply; 6+ messages in thread
From: Oliver Hartkopp @ 2010-12-26 11:29 UTC (permalink / raw)
To: Dan Rosenberg; +Cc: Urs Thuermann, David S. Miller, netdev, security
On 25.12.2010 23:41, Dan Rosenberg wrote:
>
>>
>> One minor question:
>>
>> AFAIK the inode numbers that can be found in /proc/<pid>/fd/* are in decimal
>> and not in hex, right?
>>
>> If so, you should use '%lu' instead of '%lx' in the patch.
>
> Yes, that's usually how they're expressed, but I did it this way for two
> reasons. Firstly, %lu would require another change to the buffer size,
> since the output could be up to 20 bytes long (plus another for the NULL
> terminator).
This is not *really* a good reason as the buffer size can be changed easily
within the same patch, right? ;-)
> Secondly, by expressing it as hex it avoids breaking any
> userland utilities that are expecting addresses, even if no such
> utilities exist.
I'm very sure there are no userland utilities. And if there were any, they
only would use the filename as unique identifiers and can't do anything with
the (kernel)addresses - as you already stated in the patch description. E.g.
thinking of some shell script the content of the filename is pretty useless
unless it is unique.
The recent patch for the 64 bit compatibility broke the length of the filename
anyway, so IMO there's no real reason left to write the inode number as a hex
value for the unique filename.
Indeed having it a decimal number would bring some added value as you can
reference the filename to the numbers in /proc/<pid>/fd/* easily.
Convinced? 8-)
Regards,
Oliver
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CAN: Use inode instead of kernel address for /proc file
2010-12-26 11:29 ` Oliver Hartkopp
@ 2010-12-26 16:38 ` Dan Rosenberg
0 siblings, 0 replies; 6+ messages in thread
From: Dan Rosenberg @ 2010-12-26 16:38 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Urs Thuermann, David S. Miller, netdev, security
On Sun, 2010-12-26 at 12:29 +0100, Oliver Hartkopp wrote:
> Convinced? 8-)
>
Absolutely. I didn't mean to come across as argumentative, I was just
trying to anticipate potential objections. I'll resend shortly.
Thanks,
Dan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-12-26 16:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-25 22:16 [PATCH] CAN: Use inode instead of kernel address for /proc file Dan Rosenberg
2010-12-25 22:22 ` Oliver Hartkopp
2010-12-25 22:31 ` Oliver Hartkopp
2010-12-25 22:41 ` Dan Rosenberg
2010-12-26 11:29 ` Oliver Hartkopp
2010-12-26 16:38 ` Dan Rosenberg
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).