Bug 238301 - [PATCH] handle encrypted swap in dumpon rc.d script
Summary: [PATCH] handle encrypted swap in dumpon rc.d script
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-rc (Nobody)
URL: https://reviews.freebsd.org/D34474
Keywords: patch
Depends on:
Blocks:
 
Reported: 2019-06-02 23:02 UTC by Ivan Rozhuk
Modified: 2022-03-17 01:09 UTC (History)
4 users (show)

See Also:
rozhuk.im: maintainer-feedback?


Attachments
Handle encrypted swap. (429 bytes, patch)
2019-06-02 23:02 UTC, Ivan Rozhuk
rozhuk.im: maintainer-approval?
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ivan Rozhuk 2019-06-02 23:02:20 UTC
Created attachment 204791 [details]
Handle encrypted swap.

Then dumpdev="AUTO" then rc.d/dumpon looks for first avail swap file in etc/fstab and if it exist - try to make them as dumpdev.

Typical encrypted swap:
/dev/SOME_DEV.eli none	swap	sw,late,ealgo=AES-XTS,keylen=256,sectorsize=4096 0	0

Then rc.d/dumpon check this - /dev/SOME_DEV.eli does not exist.
Even if it exist it can not be used as dumpdev die to encrypt key will be destroyed on reboot.

Patch simple strip ".eli" from end of devname before check existence.


PS:
1. Probably savecore should have some flag to able to write zeros/random on dumpdev after read dump.
2. Feature req: run "trim" for swap before "swapon".
Comment 1 Ivan Rozhuk 2020-04-16 07:53:57 UTC
maintainer timeout
Comment 2 Jamie Landeg-Jones 2020-12-22 13:32:29 UTC
As you probably know, since your original post, trim before swapon has since been added:

fstab:

/dev/ada0p1.eli none swap sw,trimonce 0 0

As for dumping to swap, doesn't that negate the purpose of using encrypted swap in the first place? All someone would need to do is force a panic, and the memory contents would be dumped in the clear.

P.S. I'm not part of the FreeBSD project, just a random user!
Comment 3 Ed Maste freebsd_committer freebsd_triage 2022-03-07 19:12:30 UTC
> As for dumping to swap, doesn't that negate the purpose of using encrypted swap
> in the first place?

It depends on one's threat model; there are certainly cases where a user would choose to use encrypted swap but still want the ability to save a core file.

We could emit a warning for this case though, something like:
---
if expr ${dev} : .*\\.eli >/dev/null; then
        echo "WARNING: kernel core dumps will be written to swap without encryption" >&2
        dev=${dev%.eli}
fi
---
Comment 4 Jamie Landeg-Jones 2022-03-08 19:39:39 UTC
(In reply to Ed Maste from comment #3)

Actually, since writing my reply, I've had an instance where I temporarily enabled dump but kept encryption!

So yes, I think your warning is an excellent idea, as it might not be totally obvious to some if they decide to encrypt swap but leave dumping enabled too.
Comment 5 Ed Maste freebsd_committer freebsd_triage 2022-03-08 19:56:29 UTC
(In reply to Jamie Landeg-Jones from comment #4)
Thanks for the comment. If you have time I'd be happy to have review or testing on the (somewhat more extensive) proposed change in https://reviews.freebsd.org/D34474
Comment 6 commit-hook freebsd_committer freebsd_triage 2022-03-10 00:47:34 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=67e751f167c98d02f85eb38401e3e6388db09ac1

commit 67e751f167c98d02f85eb38401e3e6388db09ac1
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2022-03-07 19:17:01 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2022-03-10 00:43:14 +0000

    dumpon: use underlying device if encrypted swap is in use

    /etc/rc.d/dumpon runs before /etc/rc.d/swap.  When encrypted swap is in
    use the .eli or .bde device will not exist at the time dumpon runs.

    Even if this is addressed it does not make sense to dump core to
    encrypted swap, as the encryption key will not be available after
    reboot rendering the dump useless.  Thus, for the case that dumpdev=AUTO
    and encrypted swap is in use, strip the extension and use the underlying
    device.

    Emit a warning if we are using the underlying device and the user has not
    configured dump encryption, so that the user knows that the will not be
    encrypted.

    PR:             238301
    Reported by:    Ivan Rozhuk
    Reviewed by:    jilles
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D34474

 libexec/rc/rc.d/dumpon | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
Comment 7 commit-hook freebsd_committer freebsd_triage 2022-03-17 01:09:43 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=2a719333189d9637c0997c4256e5a42a38505c1e

commit 2a719333189d9637c0997c4256e5a42a38505c1e
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2022-03-07 19:17:01 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2022-03-17 01:08:56 +0000

    dumpon: use underlying device if encrypted swap is in use

    /etc/rc.d/dumpon runs before /etc/rc.d/swap.  When encrypted swap is in
    use the .eli or .bde device will not exist at the time dumpon runs.

    Even if this is addressed it does not make sense to dump core to
    encrypted swap, as the encryption key will not be available after
    reboot rendering the dump useless.  Thus, for the case that dumpdev=AUTO
    and encrypted swap is in use, strip the extension and use the underlying
    device.

    Emit a warning if we are using the underlying device and the user has not
    configured dump encryption, so that the user knows that the will not be
    encrypted.

    PR:             238301
    Reported by:    Ivan Rozhuk
    Reviewed by:    jilles
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D34474

    (cherry picked from commit 67e751f167c98d02f85eb38401e3e6388db09ac1)

 libexec/rc/rc.d/dumpon | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)