agp: potential info leak in compat_agpioc_info_wrap()

PATCH In review Dan Carpenter 2 participants 3 messages Sep 23, 2011

Subsystems (2)

Kernel janitors Agpgart driver

lore

Share & tools

lore

Dan Carpenter

author

2011-09-23 06:19 UTC #0 lore raw

PATCH 1/1 agp: potential info leak in compat_agpioc_info_wrap()

Smatch has a new check for Rosenberg type information leaks where structs are copied to the user with uninitialized stack data in them.

The agp_info32 struct has a 4 byte hole in it so we need to clear that before copying it to userspace.

struct agp_info32 {
struct agp_version version; /* 0 0 */

/* XXX 4 bytes hole, try to pack */

u32 bridge_id; /* 4 4 */

Signed-off-by Dan Carpenter <[email protected]>

drivers/char/agp/compat_ioctl.c +2 -0

index a48e05b..0509497 100644
@@ -42,6 +42,8 @@ static int compat_agpioc_info_wrap(struct agp_file_private *priv, void __user *a
 
 	agp_copy_info(agp_bridge, &kerninfo);
 
+	memset(&userinfo, 0, sizeof(userinfo));
+
 	userinfo.version.major = kerninfo.version.major;
 	userinfo.version.minor = kerninfo.version.minor;
 	userinfo.bridge_id = kerninfo.device->vendor |

Eric Dumazet

2011-09-23 07:04 UTC #1 lore raw

Quoted 3 lines · Dan Carpenter

Smatch has a new check for Rosenberg type information leaks where structs are copied to the user with uninitialized stack data in them.

Hmm, automatic tools should not send patches without human check.

Quoted 8 lines

The agp_info32 struct has a 4 byte hole in it so we need to clear that before copying it to userspace.
struct agp_info32 {
struct agp_version version; /* 0 0 */
/* XXX 4 bytes hole, try to pack */

This makes no sense :

Why should we have a 32bit hole before an u32 field ?

Which arch mandates this ?

Quoted 17 lines

u32 bridge_id; /* 4 4 */

Signed-off-by Dan Carpenter <[email protected]>

drivers/char/agp/compat_ioctl.c +2 -0

index a48e05b..0509497 100644
@@ -42,6 +42,8 @@ static int compat_agpioc_info_wrap(struct agp_file_private *priv, void __user *a
 
 	agp_copy_info(agp_bridge, &kerninfo);
 
+	memset(&userinfo, 0, sizeof(userinfo));
+
 	userinfo.version.major = kerninfo.version.major;
 	userinfo.version.minor = kerninfo.version.minor;
 	userinfo.bridge_id = kerninfo.device->vendor |

Dan Carpenter

author

2011-09-23 07:30 UTC #2 lore raw

Quoted 6 lines · Eric Dumazet

Quoted 6 lines
The agp_info32 struct has a 4 byte hole in it so we need to clear that before copying it to userspace.
struct agp_info32 {
struct agp_version version; /* 0 0 */

Quoted 7 lines

Quoted 2 lines
        /* XXX 4 bytes hole, try to pack */
This makes no sense :
Why should we have a 32bit hole before an u32 field ?

Crap. You're right. It saw the agp_version struct as a hole.

Sorry for the noise.

regards,
dan carpenter