linux-kernel - Re: [PATCH 3/5] eCryptfs: Filename Encryption: Encoding and encryption functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 05 Nov 2008 10:17:36 -0800
From:	Dave Hansen <dave@...ux.vnet.ibm.com>
To:	Michael Halcrow <mhalcrow@...ibm.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Dustin Kirkland <dustin.kirkland@...il.com>,
	Eric Sandeen <sandeen@...hat.com>,
	Tyler Hicks <tchicks@...ibm.com>,
	David Kleikamp <shaggy@...ibm.com>
Subject: Re: [PATCH 3/5] eCryptfs: Filename Encryption: Encoding and
	encryption functions

On Tue, 2008-11-04 at 15:41 -0600, Michael Halcrow wrote:
> +static int ecryptfs_copy_filename(char **copied_name, size_t *copied_name_size,
> +                                 const char *name, size_t name_size)
> +{
> +       int rc = 0;
> +
> +       (*copied_name) = kmalloc((name_size + 2), GFP_KERNEL);
> +       if (!(*copied_name)) {
> +               rc = -ENOMEM;
> +               goto out;
> +       }
> +       memcpy((void *)(*copied_name), (void *)name, name_size);
> +       (*copied_name)[(name_size)] = '\0';     /* Only for convenience
> +                                                * in printing out the
> +                                                * string in debug
> +                                                * messages */
> +       (*copied_name_size) = (name_size + 1);
> +out:
> +       return rc;
> +}

Might this be a good place to use ERR_PTR()?  The pointer arithmetic
here looks a bit goofy.  Is this all just to get 'copied_name_size'
returned?  Why does it even matter if it is only there for printk'ing?

But, as I look at it, you do this all over the patch(es) and I think it
really reduces the readability everywhere.

If you truly need to track the actual allocated name size, I'd suggest
just having a:

struct e...c_filename {
	char *name;
	char *len;
};

Stack allocate that sucker just like you stack allocate
'copied_name_size', and then pass *it* around.

	filename->name = foo;
	filename->bar = 1234;

is way more readable than:

+                       (*encoded_name) = NULL;
+                       (*encoded_name_size) = 0;

and

+                       (*encoded_name)[(*encoded_name_size)] = '\0';
+                       (*encoded_name_size)++;

I'm certainly guilty of overly-verbose names for things, but I think:

	ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE

may be a few characters too long.  :)

This construct:

+               decoded_name = kmalloc(decoded_name_size, GFP_KERNEL);
+               if (!decoded_name) {
+                       printk(KERN_ERR "%s: Out of memory whilst attempting "
+                              "to kmalloc [%Zd] bytes\n", __func__,
+                              decoded_name_size);
+                       rc = -ENOMEM;
+                       goto out;
+               }

also appears all over.  Personally, I think this is way too verbose, but
I can also see how it would be useful.  But, the crappy part is that
there is nothing unique in each of this printk()s to help the dmesg
reader to figure out what is going on.

I think you'd save yourself a lot of code if you had an
ecryptfs_kmalloc(), did the NULL check and printk() in there, and also
added a stack dump.

>         rc = write_tag_66_packet(auth_tok->token.private_key.signature,
> -                                ecryptfs_code_for_cipher_string(crypt_stat),
> +                                ecryptfs_code_for_cipher_string(
> +                                        crypt_stat->cipher,
> +                                        crypt_stat->key_size),
>                                  crypt_stat, &payload, &payload_len);
>         if (rc) {

Why not just have ecryptfs_code_for_cipher_string() do the ->cipher and
->key_size lookup?  

-- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/