linux-apfs / linux-apfs-rw

APFS module for linux, with experimental write support
GNU General Public License v2.0
532 stars 35 forks source link

Print better error message when trying to use encryption #11

Closed Luflosi closed 1 year ago

Luflosi commented 3 years ago

When trying to mount an encrypted APFS disk, mount prints:

mount(2) system call failed: Structure needs cleaning.

and the following message appears in dmesg:

APFS (88g): bad node in block 0x27f9
APFS (88g): unable to read catalog root node

This is not a good error message as it doesn't tell the user the actual reason why this failed. I made this patch at the end of 2019 but never bothered to upstream it:

diff --git a/super.c b/super.c
index c124820..d40802d 100644
--- a/super.c
+++ b/super.c
@@ -398,6 +398,11 @@ int apfs_map_volume_super(struct super_block *sb, bool write)
        err = -EFSBADCRC;
        goto fail;
    }
+   if ((le32_to_cpu(vsb_raw->apfs_fs_flags) & 3) != APFS_FS_UNENCRYPTED) {
+       apfs_err(sb, "encrypted volumes are not supported.");
+       err = -ENOSYS;
+       goto fail;
+   }

    sbi->s_vsb_raw = vsb_raw;
    sbi->s_vobject.sb = sb;

With this patch mount prints:

mount(2) system call failed: Function not implemented.

and

APFS (88g): encrypted volumes are not supported.

appears in dmesg. I considered submitting it to the mailing list but I think this is not a good patch because I think it should be in the apfs_check_features() function and the hardcoded magic number looks ugly. There is already an error message in apfs_check_features() that prints "encrypted volumes are not supported" but the condition doesn't seem to be sufficient to catch this incompatible feature. I wonder why apfs_check_features() returns EINVAL in case of an error, instead of ENOSYS as "Function not implemented" seems more fitting than "Invalid argument" for features that are not implemented. Feel free to use this patch and fix the things I think are not good. I'm willing to test any code you may develop or send you a sample of an encrypted volume.

HLFH commented 3 years ago

Same issue.

Patronics commented 1 year ago

Hmm, I'm looking into this a bit, it looks like the flags you're checking for, are APFS_FS_UNENCRYPTED and APFS_FS_RESERVED_2, according to the [APFS Reference]. (https://developer.apple.com/support/downloads/Apple-File-System-Reference.pdf). How did you determine that both of these flags are relevant? Are you sure it's not actually just the APFS_FS_UNENCRYPTED that needs to be checked, @Luflosi?

Luflosi commented 1 year ago

No, I'm not sure. This is just what worked for me. If I remember correctly, I also took (tried to take?) inspiration from apfs-fuse but the logic was too complicated for me to fully understand in a reasonable amount of time. Feel free to improve my code and make a PR. I also think, that the code probably belongs in apfs_check_features().

Patronics commented 1 year ago

If my hypothesis is right that it's actually only the APFS_FS_UNENCRYPTED flag that we care about, this commit should fix it. As you say, I added it to apfs_check_features, mimicking the structure that was already there. I haven't tested it yet though, would appreciate if you could test it from the commit referenced above :)

Luflosi commented 1 year ago

I just looked in the apfs-fuse code again and they use if ((m_sb.apfs_fs_flags & 3) != APFS_FS_UNENCRYPTED) for some reason. This is why I wrote my patch the way I did. But I'm not sure if this is correct and in my testing your code also correctly identifies my encrypted disk, so let's just use your implementation until we find an example where that doesn't work.

eafer commented 1 year ago

I think this is fixed now, feel free to reopen if you disagree.