Skip to contentSkip to conversation

crypto: zip - Memory corruption in zip_clear_stats()

PATCH In review Dan Carpenter 4 participants 5 messages Mar 17, 2017 – Mar 24, 2017

Subsystems (2)
Kernel janitors Crypto API
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 crypto: zip - Memory corruption in zip_clear_stats()

There is a typo here. It should be "stats" instead of "state". The impact is that we clear 224 bytes instead of 80 and we zero out memory that we shouldn't.

Fixes 09ae5d37e093 ("crypto: zip - Add Compression/Decompression statistics")
Signed-off-by Dan Carpenter <[email protected]>
drivers/crypto/cavium/zip/zip_main.c +1 -1
index 0951e20b395b..6ff13d80d82e 100644
@@ -530,7 +530,7 @@ static int zip_clear_stats(struct seq_file *s, void *unused)
 	for (index = 0; index < MAX_ZIP_DEVICES; index++) {
 		if (zip_dev[index]) {
 			memset(&zip_dev[index]->stats, 0,
-			       sizeof(struct zip_state));
+			       sizeof(struct zip_stats));
 			seq_printf(s, "Cleared stats for zip %d\n", index);
 		}
 	}
#1 lore raw

Am 17.03.2017 21:46, schrieb Dan Carpenter:

Quoted 12 lines

There is a typo here. It should be "stats" instead of "state". The impact is that we clear 224 bytes instead of 80 and we zero out memory that we shouldn't.

Fixes 09ae5d37e093 ("crypto: zip - Add Compression/Decompression statistics")
Signed-off-by Dan Carpenter <[email protected]>
drivers/crypto/cavium/zip/zip_main.c +0 -0
index 0951e20b395b..6ff13d80d82e 100644

drivers/crypto/cavium/zip/zip_main.c static int zip_clear_stats(struct seq_file *s, void *unused) · line 533

Quoted 5 lines
drivers/crypto/cavium/zip/zip_main.c +1 -1
@@ -530,7 +530,7 @@ static int zip_clear_stats(struct seq_file *s, void *unused)
 	for (index = 0; index < MAX_ZIP_DEVICES; index++) {
 		if (zip_dev[index]) {
 			memset(&zip_dev[index]->stats, 0,
-			       sizeof(struct zip_state));
+			       sizeof(struct zip_stats));

as future FIXME some show find a name that differ in more than just the last char. NTL maybe
sizeof(zip_dev[index]->stats)
can be used here ?

re,
wh

Quoted 8 lines
drivers/crypto/cavium/zip/zip_main.c +0 -1
 			seq_printf(s, "Cleared stats for zip %d\n", index);
 		}
 	}
--

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

author
#2 lore raw
Quoted 10 lines · walter harms

Am 17.03.2017 21:46, schrieb Dan Carpenter:

Quoted 7 lines

There is a typo here. It should be "stats" instead of "state". The impact is that we clear 224 bytes instead of 80 and we zero out memory that we shouldn't.

Fixes 09ae5d37e093 ("crypto: zip - Add Compression/Decompression statistics")
Signed-off-by Dan Carpenter <[email protected]>
Quoted 16 lines

drivers/crypto/cavium/zip/zip_main.c static int zip_clear_stats(struct seq_file *s, void *unused) · line 533

Quoted 10 lines
drivers/crypto/cavium/zip/zip_main.c +1 -1
index 0951e20b395b..6ff13d80d82e 100644
@@ -530,7 +530,7 @@ static int zip_clear_stats(struct seq_file *s, void *unused)
 	for (index = 0; index < MAX_ZIP_DEVICES; index++) {
 		if (zip_dev[index]) {
 			memset(&zip_dev[index]->stats, 0,
-			       sizeof(struct zip_state));
+			       sizeof(struct zip_stats));

as future FIXME some show find a name that differ in more than just the last char. NTL maybe
sizeof(zip_dev[index]->stats)
can be used here ?

That's sort of unweildy. I don't fear that change because I'm confident I would catch it with static analysis.

regards,
dan carpenter

#3 lore raw
Quoted 7 lines · Dan Carpenter
Quoted 6 lines · walter harms

Am 17.03.2017 21:46, schrieb Dan Carpenter:

Quoted 3 lines

There is a typo here. It should be "stats" instead of "state". The impact is that we clear 224 bytes instead of 80 and we zero out memory that we shouldn't.

Thank you Dan for identifying the issue. Yes there is a typo and it needs a fix.

Quoted 13 lines
Quoted 13 lines

drivers/crypto/cavium/zip/zip_main.c static int zip_clear_stats(struct seq_file *s, void *unused) · line 533

Quoted 13 lines
Fixes 09ae5d37e093 ("crypto: zip - Add Compression/Decompression statistics")
Signed-off-by Dan Carpenter <[email protected]>
drivers/crypto/cavium/zip/zip_main.c +1 -1
index 0951e20b395b..6ff13d80d82e 100644
@@ -530,7 +530,7 @@ static int zip_clear_stats(struct seq_file *s, void *unused)
    for (index = 0; index < MAX_ZIP_DEVICES; index++) {
            if (zip_dev[index]) {
                    memset(&zip_dev[index]->stats, 0,
-                          sizeof(struct zip_state));
+                          sizeof(struct zip_stats));

Yes this resolves the issue.
Thanks for this fix.
Mahipal

Quoted 12 lines
Quoted 5 lines

as future FIXME some show find a name that differ in more than just the last char. NTL maybe
sizeof(zip_dev[index]->stats)
can be used here ?

That's sort of unweildy. I don't fear that change because I'm confident I would catch it with static analysis.

regards,
dan carpenter

maintainer
#4 lore raw
In reply to Dan Carpenter
Quoted 6 lines · Dan Carpenter

There is a typo here. It should be "stats" instead of "state". The impact is that we clear 224 bytes instead of 80 and we zero out memory that we shouldn't.

Fixes 09ae5d37e093 ("crypto: zip - Add Compression/Decompression statistics")
Signed-off-by Dan Carpenter <[email protected]>

Patch applied. Thanks.

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt