Reject LUKS2 header with out-of-range encryption sector size.

LUKS2 keeps the encryption sector size in the segment metadata. MAX_SECTOR_SIZE (4096 bytes, the smallest page size among the platforms cryptsetup runs on) is the largest value the format allows, because a LUKS device has to remain openable on all of them.

Writing a header enforces that, through LUKS2_check_encryption_sector(), so cryptsetup never produces one that exceeds it. Reading a header does
not: hdr_validate_crypt_segment() only requires "sector_size" to be non-zero and 512-aligned. A header carrying a larger value - after metadata corruption, or from anything that wrote it other than cryptsetup - therefore loads without complaint, and luksDump reports it as a valid LUKS device:

	sector: 65536 [bytes]

Activating it then fails, but only because dm-crypt applies a 4096-byte limit of its own. A constraint of the LUKS2 format is in effect being enforced by an implementation detail of one kernel target, which is the wrong place for it and leaves the tools reporting such a device as valid.

Check the limit where the rest of the header is checked, so that the same rule applies to a header being read as to one being written.

This cannot reject anything cryptsetup has written. LUKS1 is unaffected, its encryption sector size is always 512 bytes.

---
 lib/luks2/luks2_json_metadata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c532576..bb88846 100644
@@ -555,7 +555,7 @@ static int hdr_validate_crypt_segment(struct crypt_device *cd, json_object *jobj
 	}
 
 	sector_size = crypt_jobj_get_uint32(jobj_sector_size);
-	if (!sector_size || MISALIGNED_512(sector_size)) {
+	if (!sector_size || MISALIGNED_512(sector_size) || sector_size > MAX_SECTOR_SIZE) {
 		log_dbg(cd, "Illegal sector size: %" PRIu32, sector_size);
 		return 1;
 	}

2.34.1

Hi,

I don't think this patch is necessary. We do not check the upper bound value for sector_size in LUKS2 metadata validation code, but I believe it's on purpose.

There may be support for larger sectors in future dm-crypt. If it happens, older cryptsetup would refuse to unlock the device. Worse, it would refuse to produce even luksDump and print out information about the LUKS2 device.

Since dm-crypt contructor currently stops you from activating such device (w/ sector_size > 4096), it's ok. Metadata corruption (accidental overwrite, bitrot corruption) is detected on crypt_load due to checksums. Only deliberate change is possible and LUKS2 metadata does not protect against harmful changes. For example changing the value from 512 to 4096 bytes would make the device unusable as well.

With regards
O.

Understood, and I agree - please drop the patch.

The forward compatibility argument is the one I had not considered. A hard rejection in the validator would stop an older cryptsetup from even dumping a header that a newer dm-crypt could legitimately support, which is worse than letting the activation fail.

You are also right that corruption does not motivate it, and I should have noticed that myself: to produce a test header with sector_size 65536 I had to go through tests/generators, which recomputes the checksum. Ordinary corruption is caught by crypt_load() long before segment validation runs, exactly as you say.

Thanks for the review.

Itai