Re: mkfs.ubifs issues with mtd-utils 2.1.0

[Markus Mayer <mmayer@xxxxxxxxxxxx>]

On Sun, 31 Mar 2019 at 23:40, David Oberhollenzer
<david.oberhollenzer@xxxxxxxxxxxxx> wrote:

Hi David,

> This check was introduced in "a767dd30 mkfs.ubifs: Add encrypted symlink support".
>
> This commit is part of the fscrypt patch set. A few lines below, the extra data is
> encrypted with the path encryption functions if there is an fscrypt context.
>
> Given that having device files on disk outside an initramfs is a rather rare thing
> nowadays (devtmpfs), it appears we unfortunately didn't think of that case and
> tests subsequently didn't cover it either.

What can I say. We are special. ;-)

> > Since I am not entirely sure of the purpose of the check, I am also
> > unsure what solution to propose. Would
> >     if (S_ISREG(st->st_mode)) /* error here */
> > or
> >     if (S_ISREG(st->st_mode) || S_ISDIR(st->st_mode)) /* error here */
> > be sufficient? This would allow links, character & block devices, sockets, etc.
>
> A quick and dirty fix would be to move the check into the branch where the data
> is actually encrypted.

I can confirm that it works for me again when I do this:

$ git diff
diff --git a/ubifs-utils/mkfs.ubifs/mkfs.ubifs.c
b/ubifs-utils/mkfs.ubifs/mkfs.ubifs.c
index e0c42f36db7f..4af5250db5e6 100644
--- a/ubifs-utils/mkfs.ubifs/mkfs.ubifs.c
+++ b/ubifs-utils/mkfs.ubifs/mkfs.ubifs.c
@@ -1531,12 +1531,12 @@ static int add_inode(struct stat *st, ino_t
inum, void *data,
        ino->flags      = cpu_to_le32(use_flags);
        ino->compr_type = cpu_to_le16(c->default_compr);
        if (data_len) {
-               if (!S_ISLNK(st->st_mode))
-                       return err_msg("Expected symlink");
-
                if (!fctx) {
                        memcpy(&ino->data, data, data_len);
                } else {
+                       if (!S_ISLNK(st->st_mode))
+                               return err_msg("Expected symlink");
+
                        ret = encrypt_symlink(&ino->data, data, data_len, fctx);
                        if (ret < 0)
                                return ret;

> However, the question remains what the fscrypt branch should actually do with
> a device number in this case.

A good question. I'll have to leave the answer to those more versed
with UBIFS than me.

It may make sense to incorporate the above change for now, especially
if determining the proper behaviour for encrypting device files takes
some time. While the change doesn't fix the behaviour if it tries to
encrypt the file system, it also doesn't change the outcome. If
encryption is being used, it'll still bail, just as it does now. The
change does fix things for non-encrypted file systems, however.

Thanks,
-Markus

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/