linux-ext4 - Re: [PATCH 20/22] ext4 crypto: Add symlink encryption

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20150412052953.GK6540@thunk.org>
Date:	Sun, 12 Apr 2015 01:29:53 -0400
From:	Theodore Ts'o <tytso@....edu>
To:	Andreas Dilger <adilger@...ger.ca>
Cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>,
	jaegeuk@...nel.org, mhalcrow@...gle.com,
	Uday Savagaonkar <savagaon@...gle.com>
Subject: Re: [PATCH 20/22] ext4 crypto: Add symlink encryption

On Wed, Apr 08, 2015 at 11:58:02AM -0600, Andreas Dilger wrote:
> On Apr 2, 2015, at 4:10 PM, Theodore Ts'o <tytso@....edu> wrote:
> > +/**
> > + * For encrypted symlinks, the ciphertext length is stored at the beginning
> > + * of the string in little-endian format.
> > + */
> > +struct ext4_encrypted_symlink_data {
> > +	__le32 len;
> > +	char encrypted_path[1];
> > +} __attribute__((__packed__));
> 
> We can't have a symlink size larger than a block (or possibly PATH_MAX),
> can we?  That would allow using __le16 for the symlink length, and
> checkpatch.pl will complain about __attribute__((__packed__)) and
> request the use of __packed instead.

Yes, although it's a bit pointless since right now we don't support
fast encrypted symlinks.  What we should probably do is to switch to
using CTS, and then add fast encrypted symlinks, and then use __le16
here.

When did checkpatch start complaining about
__attribute__((__packed__))?  It's not complaining for me.

> > +static inline u32 encrypted_symlink_data_len(u32 l)
> > +{
> > +	return ((l + EXT4_CRYPTO_BLOCK_SIZE - 1) / EXT4_CRYPTO_BLOCK_SIZE)
> > +		* EXT4_CRYPTO_BLOCK_SIZE
> > +		+ sizeof(struct ext4_encrypted_symlink_data) - 1;
> 
> Coding style has operators at the end of the line instead of the start.

Thanks, fixed.

> > +		else {
> > +			sd = kmalloc(l2 + 1, GFP_NOFS);
    			   ...
> 
> Can "sd" moved inside this scope?  It doesn't appear to be used outside.

Good point, thanks.

> > diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
> > index ff37119..d788891 100644
> > --- a/fs/ext4/symlink.c
> > +++ b/fs/ext4/symlink.c
> > @@ -22,9 +22,106 @@
> > #include <linux/namei.h>
> > #include "ext4.h"
> > #include "xattr.h"
> > +#include "ext4_crypto.h"
> > 
> > +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> > static void *ext4_follow_link(struct dentry *dentry, struct nameidata *nd)
> > {
> > +	struct page *cpage = NULL;
> > +	char *caddr, *paddr;
> > +	struct ext4_str cstr, pstr;
> > +	struct inode *inode = dentry->d_inode;
> > +	struct ext4_fname_crypto_ctx *ctx = NULL;
> > +	struct ext4_encrypted_symlink_data *sd;
> > +	loff_t size = min(inode->i_size, (loff_t) PAGE_SIZE-1);
> 
> No space after typecast.  Should this use min_t() instead?

I'll change it to use min_t, thanks.

> > +		if ((cstr.len + sizeof(struct ext4_encrypted_symlink_data) - 1)
> > +			> inode->i_sb->s_blocksize) {
> 
> Operator at the end of the previous line.
> Continued line should align after '(' from previous line.

Thanks, will fix.

> > +		plen = (cstr.len < EXT4_FNAME_CRYPTO_DIGEST_SIZE*2)
> > +			? EXT4_FNAME_CRYPTO_DIGEST_SIZE*2
> > +			: cstr.len;
> 
> Operators at the end of the previous line.

Thanks, will fix.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html