Skip to contentSkip to conversation

sata_sil24: memset() overflow

PATCH Applied Dan Carpenter 3 participants 3 messages Jun 9, 2010 – Jun 10, 2010

Subsystems (2)
Kernel janitors libata
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 sata_sil24: memset() overflow

cb->atapi.cdb is an array of 16 u8 elements. The call too memset() would set the first part of the sge array to zero as well. It's not a packed struct.

This one has been around for five years. I found it with Smatch. I think the reason no one has seen it before is because we normally call sil24_fill_sg() and that overwrites sge with proper information?

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

---
I don't have this hardware myself so I can't test it. Sorry. :(

drivers/ata/sata_sil24.c +1 -1
index e925051..9e459eb 100644
@@ -865,7 +865,7 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)
 	} else {
 		prb = &cb->atapi.prb;
 		sge = cb->atapi.sge;
-		memset(cb->atapi.cdb, 0, 32);
+		memset(cb->atapi.cdb, 0, sizeof(cb->atapi.cdb));
 		memcpy(cb->atapi.cdb, qc->cdb, qc->dev->cdb_len);
 
 		if (ata_is_data(qc->tf.protocol)) {
Acked-by on v1 1/1
#1 lore raw
Quoted 7 lines · Dan Carpenter

cb->atapi.cdb is an array of 16 u8 elements. The call too memset() would set the first part of the sge array to zero as well. It's not a packed struct.

This one has been around for five years. I found it with Smatch. I think the reason no one has seen it before is because we normally call sil24_fill_sg() and that overwrites sge with proper information?

Yeah, the table is built after the memset so it doesn't really break anything but still where did that 32 come from? :-)

Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>
Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line
tejun
Applied
#2 lore raw
In reply to Dan Carpenter
Quoted 17 lines · Dan Carpenter

cb->atapi.cdb is an array of 16 u8 elements. The call too memset() would set the first part of the sge array to zero as well. It's not a packed struct.

This one has been around for five years. I found it with Smatch. I think the reason no one has seen it before is because we normally call sil24_fill_sg() and that overwrites sge with proper information?

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

---
I don't have this hardware myself so I can't test it. Sorry. :(

drivers/ata/sata_sil24.c +0 -0
index e925051..9e459eb 100644

drivers/ata/sata_sil24.c static void sil24_qc_prep(struct ata_queued_cmd *qc) · line 869

Quoted 6 lines
drivers/ata/sata_sil24.c +1 -1
@@ -865,7 +865,7 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)
  	} else {
  		prb =&cb->atapi.prb;
  		sge = cb->atapi.sge;
-		memset(cb->atapi.cdb, 0, 32);
+		memset(cb->atapi.cdb, 0, sizeof(cb->atapi.cdb));
  		memcpy(cb->atapi.cdb, qc->cdb, qc->dev->cdb_len);

applied