linux-kernel - Re: [2.6.26 PATCH, RESEND]: fs_stack/eCryptfs: fsstack_copy

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <20080430101704.9cbd6384.akpm@linux-foundation.org>
Date:	Wed, 30 Apr 2008 10:17:04 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Erez Zadok <ezk@...sunysb.edu>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	viro@...iv.linux.org.uk, hch@...radead.org, mhalcrow@...ibm.com,
	hugh@...itas.com
Subject: Re: [2.6.26 PATCH, RESEND]: fs_stack/eCryptfs: fsstack_copy_*
 updates

On Mon, 21 Apr 2008 02:50:42 -0400
Erez Zadok <ezk@...sunysb.edu> wrote:

> 
> 1. remove the 3rd arg to fsstack_copy_attr_all.  There are no users for it:
>    ecryptfs never used the 3rd arg; unionfs stopped using it a long time
>    ago.  Halcrow ok'ed this patch some time ago.
> 
> 2. add necessary locking for 32-bit smp systems in fsstack_copy_inode_size
>    (courtesy Hugh Dickins).
> 
> 3. minor commenting style changes, and addition of copyrights which were
>    missing.
> 
> Acked-by: Mike Halcrow <mhalcrow@...ibm.com>
> Signed-off-by: Erez Zadok <ezk@...sunysb.edu>
>
> ...
>
>  void fsstack_copy_inode_size(struct inode *dst, const struct inode *src)
>  {
> -	i_size_write(dst, i_size_read((struct inode *)src));
> +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
> +	spin_lock(&dst->i_lock);
> +#endif

The defined(CONFIG_SMP) is wrong.  The spinlock is here to protect
dst->i_blocks, but it can be corrupted via preemption on uniprocessor as
well.  So a plain old

#if BITS_PER_LONG == 32

would fix that.


> +	i_size_write(dst, i_size_read(src));
>  	dst->i_blocks = src->i_blocks;
> +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
> +	spin_unlock(&dst->i_lock);
> +#endif
>  }

However, what about src->i_blocks?  It is protected by src->i_lock.  The
code as you have it here could read transient values.

Furthermore, i_lock is defined as an innermost lock, for protection of
inode internals.  But here we're proposing "taking" inode->i_size_seqcount
inside i_lock.  Not necessarily a problem, but it broke the old rule.

We're also doing a read_seqlock of a _different_ inode inside this inode's
i_lock.  Again, this is not necessarily a problem (but it might be!) but it
adds complexity and needs thought.


Can we avoid having to think?

void fsstack_copy_inode_size(struct inode *dst, const struct inode *src)
{
	blkcnt_t i_blocks;
	loff_t i_size;

	i_size = i_size_read(src);
	spin_lock_32bit(&src->i_lock);
	i_blocks = src->i_blocks;
	spin_unlock_32bit(&src->i_lock);

	i_size_write(dst, i_size);
	spin_lock_32bit(&dst->i_lock)
	dst->i_blocks = i_blocks;
	spin_unlock_32bit(&dst->i_lock)
}


--
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/