Skip to contentSkip to conversation

dm raid: pointer math issue in super_sync()

PATCH In review Dan Carpenter 2 participants 4 messages Oct 21, 2014

Subsystems (3)
Kernel janitors MD RAID Device mapper
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 dm raid: pointer math issue in super_sync()

"sb" is a dm_raid_superblock struct pointer so the pointer math doesn't work and we will end up corrupting memory.

Signed-off-by Dan Carpenter <[email protected]>
drivers/md/dm-raid.c +1 -1
index b802644..a7cb9dd 100644
@@ -826,7 +826,7 @@ static void super_sync(struct mddev *mddev, struct md_rdev *rdev)
 		    test_bit(Faulty, &(rs->dev[i].rdev.flags)))
 			failed_devices |= (1ULL << i);
 
-	memset(sb + sizeof(*sb), 0, rdev->sb_size - sizeof(*sb));
+	memset(sb + 1, 0, rdev->sb_size - sizeof(*sb));
 
 	sb->magic = cpu_to_le32(DM_RAID_MAGIC);
 	sb->features = cpu_to_le32(0);	/* No features yet */
#1 lore raw
Quoted 10 lines · Dan Carpenter

"sb" is a dm_raid_superblock struct pointer so the pointer math doesn't work and we will end up corrupting memory.

Signed-off-by Dan Carpenter <[email protected]>
drivers/md/dm-raid.c +0 -0
index b802644..a7cb9dd 100644

drivers/md/dm-raid.c static void super_sync(struct mddev *mddev, struct md_rdev *rdev) · line 832

Quoted 8 lines
drivers/md/dm-raid.c +1 -1
@@ -826,7 +826,7 @@ static void super_sync(struct mddev *mddev, struct md_rdev *rdev)
 		    test_bit(Faulty, &(rs->dev[i].rdev.flags)))
 			failed_devices |= (1ULL << i);
 
-	memset(sb + sizeof(*sb), 0, rdev->sb_size - sizeof(*sb));
+	memset(sb + 1, 0, rdev->sb_size - sizeof(*sb));
 
 	sb->magic = cpu_to_le32(DM_RAID_MAGIC);
 	sb->features = cpu_to_le32(0);	/* No features yet */

Not following... sizeof(*sb) != sizeof(sb). So I'm not seeing a problem.

Nor am I seeing how you think sb + 1 is equivalent to what Heinz intended (zero the memory following the sizeof(struct dm_raid_superblock)).

author
#2 lore raw
Quoted 11 lines · Mike Snitzer

drivers/md/dm-raid.c static void super_sync(struct mddev *mddev, struct md_rdev *rdev) · line 832

Quoted 5 lines
drivers/md/dm-raid.c +1 -1
-	memset(sb + sizeof(*sb), 0, rdev->sb_size - sizeof(*sb));
+	memset(sb + 1, 0, rdev->sb_size - sizeof(*sb));
 
 	sb->magic = cpu_to_le32(DM_RAID_MAGIC);
 	sb->features = cpu_to_le32(0);	/* No features yet */

Not following... sizeof(*sb) != sizeof(sb). So I'm not seeing a problem.

Nor am I seeing how you think sb + 1 is equivalent to what Heinz intended (zero the memory following the sizeof(struct dm_raid_superblock)).

It's pointer math.

sizeof(*sb) is 512.

"sb + sizeof(*sb)" is the same as (void *)sb + 512 * 512. "sb + 1" is the same as (void *)sb + 512.

regards,
dan carpenter

#3 lore raw
Quoted 10 lines · Dan Carpenter
Quoted 9 lines · Mike Snitzer

drivers/md/dm-raid.c static void super_sync(struct mddev *mddev, struct md_rdev *rdev) · line 832

Quoted 5 lines
drivers/md/dm-raid.c +1 -1
-	memset(sb + sizeof(*sb), 0, rdev->sb_size - sizeof(*sb));
+	memset(sb + 1, 0, rdev->sb_size - sizeof(*sb));
 
 	sb->magic = cpu_to_le32(DM_RAID_MAGIC);
 	sb->features = cpu_to_le32(0);	/* No features yet */

Not following... sizeof(*sb) != sizeof(sb). So I'm not seeing a problem.

Quoted 4 lines
Quoted 2 lines

Nor am I seeing how you think sb + 1 is equivalent to what Heinz intended (zero the memory following the sizeof(struct dm_raid_superblock)).

It's pointer math.

Yes, I see that now..

Quoted 4 lines

sizeof(*sb) is 512.

"sb + sizeof(*sb)" is the same as (void *)sb + 512 * 512. "sb + 1" is the same as (void *)sb + 512.

Actually, Heinz removed the 452 bytes of padding from struct dm_raid_superblock, so it is more like:

sizeof(*sb) == sizeof(struct dm_raid_super_block) == 60

"sb + sizeof(*sb)" is the same as (void *)sb + 60 * 60 "sb + 1" is the same as (void *)sb + 60.

But regardless, your broader point on the math stands. I'll get this fixed up, thanks!

Mike