linux-kernel - Re: [PATCH v2] dm: verity target

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: <20111114175423.GA13132@google.com>
Date:	Mon, 14 Nov 2011 09:54:23 -0800
From:	Mandeep Singh Baines <msb@...omium.org>
To:	Steffen Klassert <steffen.klassert@...unet.com>
Cc:	Mandeep Singh Baines <msb@...omium.org>,
	Alasdair G Kergon <agk@...hat.com>, dm-devel@...hat.com,
	Will Drewry <wad@...omium.org>,
	Elly Jones <ellyjones@...omium.org>,
	Milan Broz <mbroz@...hat.com>,
	Olof Johansson <olofj@...omium.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] dm: verity target

Steffen Klassert (steffen.klassert@...unet.com) wrote:
> On Fri, Nov 11, 2011 at 09:34:44AM -0800, Mandeep Singh Baines wrote:
> >
> > +/**
> > + * dm_bht_compute_hash: hashes a page of data
> > + */
> > +static int dm_bht_compute_hash(struct dm_bht *bht, struct page *pg,
> > +			       unsigned int offset, u8 *digest)
> > +{
> > +	struct shash_desc *hash_desc = bht->hash_desc[smp_processor_id()];
> > +	void *data;
> > +	int err;
> 
> You don't need to have one shash_desc per cpu. As the shash interface is
> synchronous, you could either allocate a new shash_desc with every call
> to dm_bht_compute_hash, or just place the shash_desc with the context to
> the stack of this function. The easiest thing would be to add the
> shash transformation direct to dm_bht and then something like
> 
>         struct {
>                 struct shash_desc desc;        
>                 char ctx[crypto_shash_descsize(bht->tfm)];                                                
>         } hash_desc;
> 
> 	hash_desc.desc.tfm = bmt->tfm;
> 
> The intermediate and final digest values are stored to hash_desc.ctx.
> This is local, hence it's reentrant.
> 

Hi Steffen,

We use a hash per CPU to avoid allocating and freeing a context per
computation. We could allocate the context on the stack as you suggest.
However, we were worried about overflowing the stack. The context can be
quite large (close to 200 bytes for sha512).

We use a hash per CPU because there can be at most one hash in progress
on each CPU at any one time.

Regards,
Mandeep

> > +
> > +	/* Note, this is synchronous. */
> > +	if (crypto_shash_init(hash_desc)) {
> > +		DMCRIT("failed to reinitialize crypto hash (proc:%d)",
> > +			smp_processor_id());
> > +		return -EINVAL;
> > +	}
> > +	data = kmap_atomic(pg);
> > +	err = crypto_shash_update(hash_desc, data + offset, PAGE_SIZE);
> > +	kunmap_atomic(data);
> > +	if (err) {
> > +		DMCRIT("crypto_hash_update failed");
> > +		return -EINVAL;
> > +	}
> > +	if (crypto_shash_update(hash_desc, bht->salt, sizeof(bht->salt))) {
> > +		DMCRIT("crypto_hash_update failed");
> > +		return -EINVAL;
> > +	}
> > +	if (crypto_shash_final(hash_desc, digest)) {
> > +		DMCRIT("crypto_hash_final failed");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> [snip]
> 
> > + * TODO(wad): All hash storage memory is pre-allocated and freed once an
> > + * entire branch has been verified.
> > + */
> > +struct dm_bht {
> > +	/* Configured values */
> > +	int depth;  /* Depth of the tree including the root */
> > +	unsigned int block_count;  /* Number of blocks hashed */
> > +	unsigned int block_size;  /* Size of a hash block */
> > +	char hash_alg[CRYPTO_MAX_ALG_NAME];
> > +	unsigned char salt[DM_BHT_SALT_SIZE];
> > +
> > +	/* Computed values */
> > +	unsigned int node_count;  /* Data size (in hashes) for each entry */
> > +	unsigned int node_count_shift;  /* first bit set - 1 */
> > +	/* There is one per CPU so that verified can be simultaneous. */
> > +	struct shash_desc *hash_desc[NR_CPUS];  /* Container for the hash alg */
> 
> As mentioned above, you don't need to have one shash_desc per cpu.
> Replace this with a 'struct crypto_shash *tfm;' and add this transformation
> to the local hash_desc.
> 
--
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/