Bug 87717

Summary: RFE: Support for Debian's offset= in /etc/crypttab
Product: systemd Reporter: vecu.bosseur
Component: generalAssignee: systemd-bugs
Status: RESOLVED FIXED QA Contact: systemd-bugs
Severity: normal    
Priority: medium CC: martin.pitt
Version: unspecified   
Hardware: All   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments: cryptsetup: Implement offset and skip options
reproducer/test script
cryptsetup: Implement offset and skip options
cryptsetup: Implement offset and skip options

Description vecu.bosseur 2014-12-25 20:58:35 UTC
Dear Developpers,

My /etc/crypttab contains:

cryptswap1 UUID=c836dd13-1b4e-4bfb-9be5-6e5d972aa75a /dev/urandom swap,offset=2048,cipher=aes-cbc-essiv:sha256

And my /etc/fstab contains:

/dev/mapper/cryptswap1 none swap sw 0 0

And this worked fine with cryptdisks_start however the option "offset" is not understood by systemd 215. I did change init system from sysvinit to systemd, and now, after 2 reboots, I don't have any swap and my device that had UUID c836dd13-1b4e-4bfb-9be5-6e5d972aa75a has seen its start erased, and thus its UUID itself, as if I had not mentioned an offset=>>0 in crypttab.

The use case for "offset=2048" is to be able to use a UUID to identify the partition I want to have encrypted swap on.  Not using an offset=>>0 parameter would unconditionally erase the whole partition, including the portion where its UUID is stored. Using any other way to identify a partition can thus cause data loss if I reparttion my disk and forget to update /etc/crypttab.

Please make systemd understand the "offset=" paramater of /etc/crypttab.

Has this problem been addressed in a subsequent systemd version?

Note: related to debian bug #751707
( https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=751707 )

Thanks,
Vecu Bosseur
Comment 1 Zbigniew Jedrzejewski-Szmek 2014-12-26 01:37:53 UTC
It's a long-standing well-known limitation:

/* Options Debian's crypttab knows we don't:

    offset=
    skip=
    precheck=
    check=
    checkargs=
    noearly=
    loud=
    keyscript=
*/

Some of those will probably never be implemented (noearly, keyscript, loud, ...), but offset certainly should.
Comment 2 Martin Pitt 2015-04-16 11:53:47 UTC
Created attachment 115118 [details] [review]
cryptsetup: Implement offset and skip options

Simple patch.
Comment 3 Martin Pitt 2015-04-16 11:54:21 UTC
Created attachment 115119 [details]
reproducer/test script

This is the reproducer and test script which I used.
Comment 4 Zbigniew Jedrzejewski-Szmek 2015-04-16 11:57:12 UTC
I think a failure to parse those parameters should be fatal. It's just to dangerous to continue.

Also "meatadata" in description :)
Comment 5 Martin Pitt 2015-04-16 15:06:26 UTC
Created attachment 115126 [details] [review]
cryptsetup: Implement offset and skip options

Indeed! Fixed both. I tested with "offset=x8" and it now correctly aborts and doesn't touch the device. offset=1024 and no offset still continue to work.

Thanks for the review!
Comment 6 Lennart Poettering 2015-04-17 11:28:23 UTC
Hmm, please use parse_bytes() for parsing byte values.
Comment 7 Lennart Poettering 2015-04-17 11:30:20 UTC
(In reply to Lennart Poettering from comment #6)
> Hmm, please use parse_bytes() for parsing byte values.

Sorry, I meant parse_size() of course.
Comment 8 Lennart Poettering 2015-04-17 11:33:39 UTC
Also, please add the initialization of .offset and .skip into the declaration of the params variable, as initializer. i.e.:

struct crypt_params_plain params = {
        .offset = arg_offset,
        .skip = arg_skip,
};

Also, please document the new switches in crypttab(5).
Comment 9 Martin Pitt 2015-04-17 14:37:46 UTC
Created attachment 115162 [details] [review]
cryptsetup: Implement offset and skip options

parse_size() is not appropriate here, as the offset=/skip= argument is a plain natural number, and the unit is always "512 byte blocks". So allowing things like "3.3K" there would be confusing, wrong, imprecise, and incompatible with the cryptsetup project.

I added documentation, the initializer, and also incorporated warning that Zbigniew suggested on the ML.
Comment 10 Lennart Poettering 2015-04-17 14:59:36 UTC
OK, if this is not a bytes value, parse_size() is probably not a good idea. (that said, I think it's a pretty poor choice to expose this as 512byte block value...)

Anyway, looks good. Please push!
Comment 11 Martin Pitt 2015-04-17 15:53:36 UTC
http://cgit.freedesktop.org/systemd/systemd/commit/?id=4eac277367

Thanks for the review and suggestions!

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.