headers
spammed | 8 Aug 12:16

Latent bug [patch]

Hi again,

When I tested the UUID-change patch on a read-only device it would fail silently. I tracked this down to the bug described below. (It is possible you already know about this and just choose to ignore it, because as far as I can tell it has no consequences for the current version of cryptsetup.)

The function write_blockwise() will return a negative value in case of an I/O error, or the number of bytes written otherwise. The return value is then tested in LUKS_write_phdr() to detect an error:

r = write_blockwise(devfd, &convHdr, sizeof(struct luks_phdr)) < sizeof(struct luks_phdr) ? -EIO : 0;

...but if write_blockwise() returns a (small) negative error number it will be cast to a (large) positive value in the comparison, because the return value of sizeof() is unsigned (size_t). So the test will fail, and r = 0, so the error will go undetected.

This is sneaky, because for older versions of gcc, sizeof() would return an ssize_t. At some point they changed it to be ANSI compliant.

There is a similar comparison in LUKS_read_phdr. The attached patch just casts the sizeof() value to ssize_t in these two cases.

Best regards,
Jacob Nielsen 
Index: luks/keymanage.c
===================================================================
--- luks/keymanage.c	(revision 58)
+++ luks/keymanage.c	(working copy)
@@ -83,7 +83,7 @@
 		return -EINVAL; 
 	}

-	if(read_blockwise(devfd, hdr, sizeof(struct luks_phdr)) < sizeof(struct luks_phdr)) {
+	if(read_blockwise(devfd, hdr, sizeof(struct luks_phdr)) < (ssize_t)sizeof(struct luks_phdr)) {
 		r = -EIO;
 	} else if(memcmp(hdr->magic, luksMagic, LUKS_MAGIC_L)) { /* Check magic */
 		set_error(_("%s is not a LUKS partition\n"), device);
@@ -138,7 +138,7 @@
 		convHdr.keyblock[i].stripes            = htonl(hdr->keyblock[i].stripes);
 	}

-	r = write_blockwise(devfd, &convHdr, sizeof(struct luks_phdr)) < sizeof(struct luks_phdr) ? -EIO : 0;
+	r = write_blockwise(devfd, &convHdr, sizeof(struct luks_phdr)) < (ssize_t)sizeof(struct
luks_phdr) ? -EIO : 0;

 	close(devfd);
 	return r;


Index: luks/keymanage.c
===================================================================
--- luks/keymanage.c	(revision 58)
+++ luks/keymanage.c	(working copy)
@@ -83,7 +83,7 @@
 		return -EINVAL; 
 	}

-	if(read_blockwise(devfd, hdr, sizeof(struct luks_phdr)) < sizeof(struct luks_phdr)) {
+	if(read_blockwise(devfd, hdr, sizeof(struct luks_phdr)) < (ssize_t)sizeof(struct luks_phdr)) {
 		r = -EIO;
 	} else if(memcmp(hdr->magic, luksMagic, LUKS_MAGIC_L)) { /* Check magic */
 		set_error(_("%s is not a LUKS partition\n"), device);
@@ -138,7 +138,7 @@
 		convHdr.keyblock[i].stripes            = htonl(hdr->keyblock[i].stripes);
 	}

-	r = write_blockwise(devfd, &convHdr, sizeof(struct luks_phdr)) < sizeof(struct luks_phdr) ? -EIO : 0;
+	r = write_blockwise(devfd, &convHdr, sizeof(struct luks_phdr)) < (ssize_t)sizeof(struct
luks_phdr) ? -EIO : 0;

 	close(devfd);
 	return r;
Permalink | Reply |

Gmane