char: synclink: fix information leak to userland
Subsystems (2)
PATCH 6/8 char: synclink: fix information leak to userland
Structure new_line is copied to userland with some padding fields unitialized. It leads to leaking of stack memory.
---
Compile tested.
drivers/char/synclink.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/char/synclink.c +2 -0
@@ -7846,6 +7846,8 @@ static int hdlcdev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)if (cmd !=SIOCWANDEV )return hdlc_ioctl (dev, ifr, cmd); +memset (&new_line,0 , size); +switch (ifr->ifr_settings.type) {case IF_GET_IFACE :/* return current sync_serial_settings */
Signature · 1 line
1.7.0.4
Quoted 2 lines · Vasiliy Kulikov
Structure
new_lineis copied to userland with some padding fields unitialized. It leads to leaking of stack memory.
I think your tool has a bug. I must admit I fail to see the padding which would cause leaks. Could you elaborate?
Quoted 20 lines
Signed-off-by Vasiliy Kulikov <[email protected]>---
Compile tested.drivers/char/synclink.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)drivers/char/synclink.c +2
-0@@ -7846,6 +7846,8 @@ static int hdlcdev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)if (cmd !=SIOCWANDEV )return hdlc_ioctl (dev, ifr, cmd); +memset (&new_line,0 , size); +switch (ifr->ifr_settings.type) {case IF_GET_IFACE :/* return current sync_serial_settings */
Signature · 2 lines
js suse labs
Quoted 6 lines · Jiri Slaby
Quoted 2 lines · Vasiliy Kulikov
Structure
new_lineis copied to userland with some padding fields unitialized. It leads to leaking of stack memory.I think your tool has a bug. I must admit I fail to see the padding which would cause leaks. Could you elaborate?
I didn't use any tool except "grep copy_to_user" :)
typedef struct {
unsigned int clock_rate; /* bits per second */
unsigned int clock_type; /* internal, external, TX-internal etc. */
unsigned short loopback;
} sync_serial_settings; /* V.35, V.24, X.21 */On x86_64 sizeof(sync_serial_settings) = 4 + 4 + 2 + 2 = 12. The last 2 is padding.
Quoted 26 lines
Quoted 21 lines
Signed-off-by Vasiliy Kulikov <[email protected]>---
Compile tested.drivers/char/synclink.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)drivers/char/synclink.c +2
-0@@ -7846,6 +7846,8 @@ static int hdlcdev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)if (cmd !=SIOCWANDEV )return hdlc_ioctl (dev, ifr, cmd); +memset (&new_line,0 , size); +switch (ifr->ifr_settings.type) {case IF_GET_IFACE :/* return current sync_serial_settings */ Signature · 2 lines
js suse labs
Signature · 1 line
Vasiliy
Quoted 10 lines · Vasiliy Kulikov
Quoted 6 lines · Jiri Slaby
Quoted 2 lines · Vasiliy Kulikov
Structure
new_lineis copied to userland with some padding fields unitialized. It leads to leaking of stack memory.I think your tool has a bug. I must admit I fail to see the padding which would cause leaks. Could you elaborate?
I didn't use any tool except "grep
copy_to_user" :)
It seems like you should be able to use pahole to make a list of structs with padding and then a checker script to find places where information is leaked.
Also someone complained to me about when I added a memset() in a fast path. The thought was that it might be faster to just initialize it instead like:
struct foo bar = {};In my case just using the initializer made the code cleaner so I did it, but neither of us actually benchmarked it.
regards,
dan carpenter
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Vasiliy Kulikov
Quoted 6 lines · Jiri Slaby
Quoted 2 lines · Vasiliy Kulikov
Structure
new_lineis copied to userland with some padding fields unitialized. It leads to leaking of stack memory.I think your tool has a bug. I must admit I fail to see the padding which would cause leaks. Could you elaborate?
Quoted 6 lines
Quoted 2 lines
I didn't use any tool except "grep
copy_to_user" :)It seems like you should be able to use pahole to make a list of structs with padding and then a checker script to find places where information is leaked.
Not all of these patches fix only padding zeroing, some of them fix uninitialized fields. One struct has partly initialized array.
Quoted 2 lines
Also someone complained to me about when I added a
memset()in a fast path.
All these cases are ioctl() handlers or similar. I don't think ioctl() should be so fast to become significantly slower with single memset().
Quoted 8 lines
The thought was that it might be faster to just initialize it instead like:
struct foo bar = {};In my case just using the initializer made the code cleaner so I did it, but neither of us actually benchmarked it.
Quoted 6 lines
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks,
Signature · 1 line
Vasiliy
Quoted 10 lines · Vasiliy Kulikov
Quoted 6 lines · Jiri Slaby
Quoted 2 lines · Vasiliy Kulikov
Structure
new_lineis copied to userland with some padding fields unitialized. It leads to leaking of stack memory.I think your tool has a bug. I must admit I fail to see the padding which would cause leaks. Could you elaborate?
I didn't use any tool except "grep
copy_to_user" :)
Quoted 8 lines
typedef struct { unsigned int clock_rate; /* bits per second */ unsigned int clock_type; /* internal, external, TX-internal etc. */ unsigned short loopback; } sync_serial_settings; /* V.35, V.24, X.21 */On
x86_64sizeof(sync_serial_settings)= 4 + 4 + 2 + 2 = 12. The last 2 is padding.
Ah, good to know that even end of structures is padded.
thanks for clarification,
Signature · 2 lines
js suse labs
Quoted 17 lines · Jiri Slaby
Quoted 16 lines · Vasiliy Kulikov
Quoted 6 lines · Jiri Slaby
Quoted 2 lines · Vasiliy Kulikov
Structure
new_lineis copied to userland with some padding fields unitialized. It leads to leaking of stack memory.I think your tool has a bug. I must admit I fail to see the padding which would cause leaks. Could you elaborate?
I didn't use any tool except "grep
copy_to_user" :)typedef struct { unsigned int clock_rate; /* bits per second */ unsigned int clock_type; /* internal, external, TX-internal etc. */ unsigned short loopback; } sync_serial_settings; /* V.35, V.24, X.21 */
Quoted 4 lines
Quoted 2 lines
On
x86_64sizeof(sync_serial_settings)= 4 + 4 + 2 + 2 = 12. The last 2 is padding.Ah, good to know that even end of structures is padded.
The rule here is that array of struct has no padding between struct elements. So, struct has padding in the end to have good alignment of the first field of the next array element.
Thanks,
Signature · 1 line
Vasiliy