Merge lp:~jderose/ecryptfs/fix-1597154 into lp:ecryptfs

[Jason Gerard DeRose]

In ecryptfs-setup-swap on line 169, the $drive and $partno are currently built from $swap like this:

drive="${swap%[0-9]*}"
partno="${swap#$drive}"

But this is incorrect for /dev/nvme* and /dev/mmcblk* drives.

For example, when $swap is "/dev/nvme0n1p3", you get a correct $partno of "3", but an incorrect $drive of "/dev/nvme0n1p" (it should be "/dev/nvme0n1", with no "p" at the end).

To fix this I added a conditional to build $drive differently when $swap matches "^/dev/nvme*|^/dev/mmcblk*", but I didn't change how $partno is built.

To verify the logic I wrote a little (out-of-band) test script:

"""
#!/bin/sh -e

test_fix() {
    swap=$1
    drive="${swap%[0-9]*}"
    partno="${swap#$drive}"
    if echo $swap | grep -qE "^/dev/nvme*|^/dev/mmcblk*"; then
        drive="${swap%p[0-9]*}"
    fi
    echo "$swap\t$drive\t$partno"
}

test_fix /dev/sda3
test_fix /dev/vda3
test_fix /dev/nvme0n1p3
test_fix /dev/mmcblk0p3
"""

Which correctly outputs:

/dev/sda3 /dev/sda 3
/dev/vda3 /dev/vda 3
/dev/nvme0n1p3 /dev/nvme0n1 3
/dev/mmcblk0p3 /dev/mmcblk0 3

I also tested this by running ecryptfs-setup-swap (from the source tree) on a system with an NVMe drive that previously hadn't had encrypted swap or encrypted home directory setup. With my change, ecryptfs-setup-swap now correctly marks the GPT swap partition as non-auto-mounting.

[Tyler Hicks (tyhicks)]

Hi Jason - Thank you for the merge proposal! I see two issues that I think should be addressed:

1) This change is brittle in that if another drive type comes along that has the foo0p0 format, this same bug will be reintroduced. Can you come up with a way to manipulate the drive path so that it handles the situation in a generic way without the need for the `echo $swap | grep -qE "^/dev/nvme*|^/dev/mmcblk*"` conditional?

2) The (existing) logic doesn't handle partition numbers with multiple digits. This can be seen by adding "test_fix /dev/mmcblk0p13" to your test script.

Thanks again!

review: Needs Fixing

[lp:~jderose/ecryptfs/fix-1597154]

883. By Jason Gerard DeRose on 2016-07-03

Tweak fix based on review from Tyler Hicks

[Jason Gerard DeRose (jderose)]

Tyler,

Okay, resubmitting with your two issues addressed:

1) I didn't come up with a unified sed expression I liked for building $drive, so I'm still using a conditional, but this time with:

  grep -qE "^/dev/.+[0-9]+p[0-9]+$"

This means the code now correctly handles the generic /dev/foo0p1 pattern. I agree that this is a better approach, although I still feel there is a certain fragility here as we're still making some assumption that we don't know for sure will hold for future types of block devices that merely match this naming pattern.

2) Nice catch! I didn't notice that the original code didn't correctly handle multi-digit partition numbers. In all cases, I'm now building $partno with:

  partno=$(echo $swap | sed "s/.\+[^0-9]\([0-9]\+\)/\1/")

I also updated my (out-of-band) test script, which now correctly handles all the key corner cases for existing types of block devices that I can think of:

"""
#!/bin/sh -e

test_fix() {
    swap=$1
    if echo $swap | grep -qE "^/dev/.+[0-9]+p[0-9]+$"; then
        drive=$(echo $swap | sed "s/\(.\+[0-9]\)p[0-9]\+/\1/")
    else
        drive=$(echo $swap | sed "s/\(.\+[^0-9]\)[0-9]\+/\1/")
    fi
    partno=$(echo $swap | sed "s/.\+[^0-9]\([0-9]\+\)/\1/")
    echo "$swap\t$drive\t$partno"
}

test_fix "/dev/sda3"
test_fix "/dev/sda33"
test_fix "/dev/sdp3"
test_fix "/dev/sdp33"

echo ""
test_fix "/dev/vda3"
test_fix "/dev/vda33"
test_fix "/dev/vdp3"
test_fix "/dev/vdp33"

echo ""
test_fix "/dev/mmcblk0p3"
test_fix "/dev/mmcblk0p33"
test_fix "/dev/mmcblk99p3"
test_fix "/dev/mmcblk99p33"

echo ""
test_fix "/dev/nvme0n1p3"
test_fix "/dev/nvme0n1p33"
test_fix "/dev/nvme99n1p3"
test_fix "/dev/nvme99n1p33"

echo ""
test_fix "/dev/nvme0n11p3"
test_fix "/dev/nvme0n11p33"
test_fix "/dev/nvme99n11p3"
test_fix "/dev/nvme99n11p33"
"""

Please let me know if I missed anything or if you'd like further tweaks!

review: Needs Resubmitting

[lp:~jderose/ecryptfs/fix-1597154]

884. By Jason Gerard DeRose on 2016-07-03

Replace $swap --> "$swap" thanks to https://www.shellcheck.net/

[Tyler Hicks (tyhicks)]

Hi Jason - the code changes look good. I made a small change to replace the sed regexes special '/' characters with ':' characters to make things a little more readable (the other call to sed in ecryptfs-setup-swap does that, as well).

That said, manual testing is not working as expected. I am testing on a freshly installed amd64 VM that was installed using GPT partitioning on an nvme drive. QEMU is being started like so:

$ qemu-system-x86_64 -enable-kvm -m 1G -vga qxl \
  -drive if=pflash,format=raw,readonly,file=/usr/share/OVMF/OVMF_CODE.fd \
  -drive if=pflash,format=raw,file=OVMF_VARS.fd \
  -drive if=none,format=qcow2,id=nvme0,file=drive.qcow2 \
  -device nvme,drive=nvme0,serial=1234

After running `sudo ecryptfs-setup-swap` and rebooting, I'm seeing /dev/mapper/cryptswap1 being populated but `swapon -s` and `free` shows that no swap partitions are enabled. If I run `sudo swapon -a` then the swap partition is enabled.

I'm trying to debug what's going on here but thought I'd mention it to you to see if you have gotten different results.

review: Needs Information

[Tyler Hicks (tyhicks)]

I take that back. I've successfully tested this patch in several fresh VMs since my last comment. I think that I may have installed my locally built ecryptfs-utils debs in a way that resulted in cryptsetup (a recommends) not being installed. This looks good to me now.

review: Approve

=== modified file 'src/utils/ecryptfs-setup-swap'
--- src/utils/ecryptfs-setup-swap	2015-08-04 15:42:02 +0000
+++ src/utils/ecryptfs-setup-swap	2016-07-03 19:05:42 +0000
@@ -166,8 +166,14 @@
 	# If this is a GPT partition, mark it as no-auto mounting, to avoid
 	# auto-activating it on boot
 	if [ "$(blkid -p -s PART_ENTRY_SCHEME -o value "$swap")" = "gpt" ]; then
-		drive="${swap%[0-9]*}"
-		partno="${swap#$drive}"
+		# Correctly handle NVMe/MMC drives, as well as any similar physical
+		# block device that follow the "/dev/foo0p1" pattern (LP: #1597154)
+		if echo "$swap" | grep -qE "^/dev/.+[0-9]+p[0-9]+$"; then
+			drive=$(echo "$swap" | sed "s/\(.\+[0-9]\)p[0-9]\+/\1/")
+		else
+			drive=$(echo "$swap" | sed "s/\(.\+[^0-9]\)[0-9]\+/\1/")
+		fi
+		partno=$(echo "$swap" | sed "s/.\+[^0-9]\([0-9]\+\)/\1/")
 		if [ -b "$drive" ]; then
 			if printf "x\np\n" | fdisk "$drive" | grep -q "^$swap .* GUID:.*\b63\b"; then
 				echo "$swap is already marked as no-auto"