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

Proposed by Jason Gerard DeRose on 2016-06-29
Status: Merged
Merged at revision: 882
Proposed branch: lp:~jderose/ecryptfs/fix-1597154
Merge into: lp:ecryptfs
Diff against target: 20 lines (+8/-2)
1 file modified
src/utils/ecryptfs-setup-swap (+8/-2)
To merge this branch: bzr merge lp:~jderose/ecryptfs/fix-1597154
Reviewer Review Type Date Requested Status
Tyler Hicks 2016-06-29 Approve on 2016-07-07
Jason Gerard DeRose (community) Resubmit on 2016-07-03
Review via email: mp+298591@code.launchpad.net

Description of the Change

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.

To post a comment you must log in.
Tyler Hicks (tyhicks) wrote :

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 updated on 2016-07-03
883. By Jason Gerard DeRose on 2016-07-03

Tweak fix based on review from Tyler Hicks

Jason Gerard DeRose (jderose) wrote :

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: Resubmit
lp:~jderose/ecryptfs/fix-1597154 updated on 2016-07-03
884. By Jason Gerard DeRose on 2016-07-03

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

Tyler Hicks (tyhicks) wrote :

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) wrote :

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

Preview Diff

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

Subscribers

People subscribed via source and target branches