Skip to contentSkip to conversation

dm crypt: remove an impossible condition

PATCH In review Dan Carpenter 2 participants 3 messages Mar 17, 2017 – Mar 18, 2017

Subsystems (3)
Kernel janitors MD RAID Device mapper
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 dm crypt: remove an impossible condition

Static checkers complain that it doesn't make sense to check if "sval" is NULL. The intention was to check if strchr() returned NULL, but in that situation "sval" would be "NULL + 1" so the check doesn't work. We know from the sscanf() that there is a ':' character in the string so the check is unnecessary and can be removed.

Now that the check doesn't depend on "sval" it can be moved earlier for readability.

Signed-off-by Dan Carpenter <[email protected]>
drivers/md/dm-crypt.c +3 -3
index ec09bd703b7d..eba218737bdb 100644
@@ -2401,12 +2401,12 @@ static int crypt_ctr_optional(struct dm_target *ti, unsigned int argc, char **ar
 		else if (!strcasecmp(opt_string, "submit_from_crypt_cpus"))
 			set_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags);
 		else if (sscanf(opt_string, "integrity:%u:", &val) == 1) {
-			cc->on_disk_tag_size = val;
-			sval = strchr(opt_string + strlen("integrity:"), ':') + 1;
-			if (val == 0 || val > MAX_TAG_SIZE || !sval) {
+			if (val == 0 || val > MAX_TAG_SIZE) {
 				ti->error = "Invalid integrity arguments";
 				return -EINVAL;
 			}
+			cc->on_disk_tag_size = val;
+			sval = strchr(opt_string + strlen("integrity:"), ':') + 1;
 			if (!strcasecmp(sval, "aead")) {
 				set_bit(CRYPT_MODE_INTEGRITY_AEAD, &cc->cipher_flags);
 			} else  if (!strncasecmp(sval, "hmac(", strlen("hmac("))) {
#1 lore raw
Quoted 10 lines · Dan Carpenter

Static checkers complain that it doesn't make sense to check if "sval" is NULL. The intention was to check if strchr() returned NULL, but in that situation "sval" would be "NULL + 1" so the check doesn't work. We know from the sscanf() that there is a ':' character in the string so the check is unnecessary and can be removed.

Now that the check doesn't depend on "sval" it can be moved earlier for readability.

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

Thanks Dan, I've folded this fix into the original commit, see:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.12&id=cf22cd5f3afe7335afee5659f7450000e8fa2a15

I didn't add your Signed-off-by though, I can backfill that if you like.. it is just I didn't want to taint you with all the extensive changes in that commit.

Mike

author
#2 lore raw
Quoted 12 lines · Mike Snitzer
Quoted 9 lines · Dan Carpenter

Static checkers complain that it doesn't make sense to check if "sval" is NULL. The intention was to check if strchr() returned NULL, but in that situation "sval" would be "NULL + 1" so the check doesn't work. We know from the sscanf() that there is a ':' character in the string so the check is unnecessary and can be removed.

Now that the check doesn't depend on "sval" it can be moved earlier for readability.

Quoted 8 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>

Thanks Dan, I've folded this fix into the original commit, see:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.12&id=cf22cd5f3afe7335afee5659f7450000e8fa2a15

I didn't add your Signed-off-by though, I can backfill that if you like.. it is just I didn't want to taint you with all the extensive changes in that commit.

That's fine, thanks. Sign-off isn't really the appropriate tag anyway...

regards,
dan carpenter