Skip to contentSkip to conversation

drivers: mtd: mtdswap: fix possible null dereference

PATCH Applied maxin 2 participants 11 messages May 18, 2011 – May 20, 2011 Memory technology devices (MTD)

lore
Share & tools
lore

maxin

author
#0 lore raw

PATCH 1/1 drivers: mtd: mtdswap: fix possible null dereference

This patch fixes the possible null dereference of the variable "oinfo" Thanks to Coverity for spotting this.

Signed-off-by Maxin B. John <[email protected]>

---

drivers/mtd/mtdswap.c +6 -1
index fed215c..863987c 100644
@@ -1450,7 +1450,12 @@ static void mtdswap_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 	}
 
 	oinfo = mtd->ecclayout;
-	if (!mtd->oobsize || !oinfo || oinfo->oobavail < MTDSWAP_OOBSIZE) {
+	if (!oinfo) {
+		printk(KERN_ERR "Not enough free bytes available for ECC");
+		return;
+	}
+
+	if (!mtd->oobsize || oinfo->oobavail < MTDSWAP_OOBSIZE) {
 		printk(KERN_ERR "%s: Not enough free bytes in OOB, "
 			"%d available, %zu needed.\n",
 			MTDSWAP_PREFIX, oinfo->oobavail, MTDSWAP_OOBSIZE);
#1 lore raw
Quoted 10 lines · maxin

This patch fixes the possible null dereference of the variable "oinfo" Thanks to Coverity for spotting this.

Signed-off-by Maxin B. John <[email protected]>

---

drivers/mtd/mtdswap.c +0 -0
index fed215c..863987c 100644

drivers/mtd/mtdswap.c static void mtdswap_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd) · line 1456

Quoted 8 lines
drivers/mtd/mtdswap.c +4 -1
@@ -1450,7 +1450,12 @@ static void mtdswap_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 	}
 
 	oinfo = mtd->ecclayout;
-	if (!mtd->oobsize || !oinfo || oinfo->oobavail < MTDSWAP_OOBSIZE) {
+	if (!oinfo) {
+		printk(KERN_ERR "Not enough free bytes available for ECC");
+		return;
+	}

Does this print makes sense? Which ECC? I think it should me something like "mtd% does not have OOB", mtd->index.

Moreover, you should make the print consistent with the other prints which start with MTDSWAP_PREFIX.

Signature · 2 lines
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
author
#2 lore raw

Hi Artem,

Quoted 6 lines · ext Artem Bityutskiy
Quoted 1 line · maxin

This patch fixes the possible null dereference of the variable "oinfo"

Does this print makes sense? Which ECC? I think it should me something like "mtd% does not have OOB", mtd->index.

Thank you very much for reviewing the patch.I have updated the print message as per your suggestion as it makes more sense for this case.

Quoted 3 lines

Moreover, you should make the print consistent with the other prints which start with MTDSWAP_PREFIX.

I have updated the patch as per your comments.

Signed-off-by Maxin B. John <[email protected]>

---

drivers/mtd/mtdswap.c +7 -1
index fed215c..62ddb3a 100644
@@ -1450,7 +1450,13 @@ static void mtdswap_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 	}
 
 	oinfo = mtd->ecclayout;
-	if (!mtd->oobsize || !oinfo || oinfo->oobavail < MTDSWAP_OOBSIZE) {
+	if (!oinfo) {
+		printk(KERN_ERR "%s: mtd%d does not have OOB"
+			"\n", MTDSWAP_PREFIX, mtd->index);
+		return;
+	}
+
+	if (!mtd->oobsize || oinfo->oobavail < MTDSWAP_OOBSIZE) {
 		printk(KERN_ERR "%s: Not enough free bytes in OOB, "
 			"%d available, %zu needed.\n",
 			MTDSWAP_PREFIX, oinfo->oobavail, MTDSWAP_OOBSIZE);
#3 lore raw
Quoted 13 lines · maxin
Quoted 3 lines

Moreover, you should make the print consistent with the other prints which start with MTDSWAP_PREFIX.

I have updated the patch as per your comments.

Signed-off-by Maxin B. John <[email protected]>

---

drivers/mtd/mtdswap.c +0 -0
index fed215c..62ddb3a 100644

drivers/mtd/mtdswap.c static void mtdswap_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd) · line 1454

Quoted 6 lines
drivers/mtd/mtdswap.c +2 -1
@@ -1450,7 +1450,13 @@ static void mtdswap_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 	}
 
 	oinfo = mtd->ecclayout;
-	if (!mtd->oobsize || !oinfo || oinfo->oobavail < MTDSWAP_OOBSIZE) {
+	if (!oinfo) {
+		printk(KERN_ERR "%s: mtd%d does not have OOB"

Sorry, but if I save this e-mail and feed it to "git am", the commit message will contain the discussion (with quotes, etc), not the nice original commit message. Would you please, send a patch which I can just save and "git am"?

Signature · 2 lines
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
author
#4 lore raw

This patch fixes the possible null dereference of the variable "oinfo" Thanks to Coverity for spotting this.

Signed-off-by Maxin B. John <[email protected]>

---

drivers/mtd/mtdswap.c +7 -1
index fed215c..62ddb3a 100644
@@ -1450,7 +1450,13 @@ static void mtdswap_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 	}
 
 	oinfo = mtd->ecclayout;
-	if (!mtd->oobsize || !oinfo || oinfo->oobavail < MTDSWAP_OOBSIZE) {
+	if (!oinfo) {
+		printk(KERN_ERR "%s: mtd%d does not have OOB"
+			"\n", MTDSWAP_PREFIX, mtd->index);
+		return;
+	}
+
+	if (!mtd->oobsize || oinfo->oobavail < MTDSWAP_OOBSIZE) {
 		printk(KERN_ERR "%s: Not enough free bytes in OOB, "
 			"%d available, %zu needed.\n",
 			MTDSWAP_PREFIX, oinfo->oobavail, MTDSWAP_OOBSIZE);
#5 lore raw
Quoted 10 lines · maxin

This patch fixes the possible null dereference of the variable "oinfo" Thanks to Coverity for spotting this.

Signed-off-by Maxin B. John <[email protected]>

---

drivers/mtd/mtdswap.c +0 -0
index fed215c..62ddb3a 100644

drivers/mtd/mtdswap.c static void mtdswap_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd) · line 1455

Quoted 7 lines
drivers/mtd/mtdswap.c +3 -1
@@ -1450,7 +1450,13 @@ static void mtdswap_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 	}
 
 	oinfo = mtd->ecclayout;
-	if (!mtd->oobsize || !oinfo || oinfo->oobavail < MTDSWAP_OOBSIZE) {
+	if (!oinfo) {
+		printk(KERN_ERR "%s: mtd%d does not have OOB"
+			"\n", MTDSWAP_PREFIX, mtd->index);

Sorry for not noticing this immediately, but we never split the strings if it is not really necessary. What is the fundamental reason to put "\n" separately?

Signature · 2 lines
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
author
#6 lore raw

Hi Artem,

Quoted 6 lines · ext Artem Bityutskiy

On Fri, 2011-05-20 at 14:22 +0100, maxin wrote:

Sorry for not noticing this immediately, but we never split the strings if it is not really necessary. What is the fundamental reason to put "\n" separately?

Thanks for a thorough review. I will modify it accordingly.

Quoted 2 lines

Also, when I "git am" your patch, the Author's name will be "maxin", is this what you want?

Hmm.. I haven't noticed this before. Thanks... I have updated the MUA settings.

Best Regards,
Maxin

#7 lore raw
In reply to maxin
Quoted 4 lines · maxin

This patch fixes the possible null dereference of the variable "oinfo" Thanks to Coverity for spotting this.

Signed-off-by Maxin B. John <[email protected]>

Also, when I "git am" your patch, the Author's name will be "maxin", is this what you want?

Signature · 2 lines
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
author
#8 lore raw

This patch fixes the possible null dereference of the variable "oinfo" Thanks to Coverity for spotting this.

Signed-off-by Maxin B. John <[email protected]>

---

drivers/mtd/mtdswap.c +7 -1
index fed215c..fd78853 100644
@@ -1450,7 +1450,13 @@ static void mtdswap_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 	}
 
 	oinfo = mtd->ecclayout;
-	if (!mtd->oobsize || !oinfo || oinfo->oobavail < MTDSWAP_OOBSIZE) {
+	if (!oinfo) {
+		printk(KERN_ERR "%s: mtd%d does not have OOB\n",
+			MTDSWAP_PREFIX, mtd->index);
+		return;
+	}
+
+	if (!mtd->oobsize || oinfo->oobavail < MTDSWAP_OOBSIZE) {
 		printk(KERN_ERR "%s: Not enough free bytes in OOB, "
 			"%d available, %zu needed.\n",
 			MTDSWAP_PREFIX, oinfo->oobavail, MTDSWAP_OOBSIZE);
Applied
#9 lore raw
Quoted 4 lines · Maxin B. John

This patch fixes the possible null dereference of the variable "oinfo" Thanks to Coverity for spotting this.

Signed-off-by Maxin B. John <[email protected]>

Pushed to l2-mtd-2.6.git, thanks.

Signature · 2 lines
Best Regards,
Artem Bityutskiy (Артём Битюцкий)