linux-ima-devel — Discussion list for problems or development.

[Linux-ima-devel] [PATCH v4 13/19] cifs: move to generic async completion

[Gilad Ben-Y. <gi...@be...>]

cifs starts an async. crypto op and waits for their completion.
Move it over to generic code doing the same.

Signed-off-by: Gilad Ben-Yossef <gi...@be...>
Acked-by: Pavel Shilovsky <ps...@mi...>
---
 fs/cifs/smb2ops.c | 30 ++++--------------------------
 1 file changed, 4 insertions(+), 26 deletions(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index cfacf2c..16fb041 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1878,22 +1878,6 @@ init_sg(struct smb_rqst *rqst, u8 *sign)
 	return sg;
 }
 
-struct cifs_crypt_result {
-	int err;
-	struct completion completion;
-};
-
-static void cifs_crypt_complete(struct crypto_async_request *req, int err)
-{
-	struct cifs_crypt_result *res = req->data;
-
-	if (err == -EINPROGRESS)
-		return;
-
-	res->err = err;
-	complete(&res->completion);
-}
-
 static int
 smb2_get_enc_key(struct TCP_Server_Info *server, __u64 ses_id, int enc, u8 *key)
 {
@@ -1934,12 +1918,10 @@ crypt_message(struct TCP_Server_Info *server, struct smb_rqst *rqst, int enc)
 	struct aead_request *req;
 	char *iv;
 	unsigned int iv_len;
-	struct cifs_crypt_result result = {0, };
+	DECLARE_CRYPTO_WAIT(wait);
 	struct crypto_aead *tfm;
 	unsigned int crypt_len = le32_to_cpu(tr_hdr->OriginalMessageSize);
 
-	init_completion(&result.completion);
-
 	rc = smb2_get_enc_key(server, tr_hdr->SessionId, enc, key);
 	if (rc) {
 		cifs_dbg(VFS, "%s: Could not get %scryption key\n", __func__,
@@ -1999,14 +1981,10 @@ crypt_message(struct TCP_Server_Info *server, struct smb_rqst *rqst, int enc)
 	aead_request_set_ad(req, assoc_data_len);
 
 	aead_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
-				  cifs_crypt_complete, &result);
+				  crypto_req_done, &wait);
 
-	rc = enc ? crypto_aead_encrypt(req) : crypto_aead_decrypt(req);
-
-	if (rc == -EINPROGRESS || rc == -EBUSY) {
-		wait_for_completion(&result.completion);
-		rc = result.err;
-	}
+	rc = crypto_wait_req(enc ? crypto_aead_encrypt(req)
+				: crypto_aead_decrypt(req), &wait);
 
 	if (!rc && enc)
 		memcpy(&tr_hdr->Signature, sign, SMB2_SIGNATURE_SIZE);
-- 
2.1.4

Re: [Linux-ima-devel] [PATCH v4 2/5] ima: use fs method to read integrity data [updated]

[Jan K. <ja...@su...>]

On Mon 07-08-17 16:12:51, Mimi Zohar wrote:
> On Mon, 2017-08-07 at 12:04 +0200, Jan Kara wrote:
> > > For DAX, unlike do_blockdev_direct_IO() which takes the lock, reading
> > > the file with O_DIRECT is fine, as dax_iomap_rw() only checks that the
> > > lock has been taken.  Assuming the file system is mounted with
> > > i_version, the file hash is updated properly.
> > 
> > Yes, for DAX direct IO is basically no different but frankly I would just
> > refuse O_DIRECT on DAX inodes as well just for the consistency sake.
> 
> Ok.  So I shouldn't revert the original commit, which fails the
> O_DIRECT open for either the buffered read or DAX.  I'll just move the
> code to a bit later, so that the failure is added to the measurement
> list.
> 
> The original commit returned -EACCES.  On xfs, the open for direct IO
> buffer read fails with -EINVAL.  Do you have a preference IMA should
> return?

Not really. -EINVAL is more traditional when direct IO is not supported but
since IMA denies access to the file, -EACCES makes sense as well.

								Honza
-- 
Jan Kara <ja...@su...>
SUSE Labs, CR

Re: [Linux-ima-devel] [PATCH v4 2/5] ima: use fs method to read integrity data [updated]

[Mimi Z. <zo...@li...>]

On Mon, 2017-08-07 at 12:04 +0200, Jan Kara wrote:
> On Fri 04-08-17 17:07:11, Mimi Zohar wrote:
> > On Thu, 2017-08-03 at 12:56 +0200, Jan Kara wrote:
> > > On Wed 02-08-17 13:11:52, Mimi Zohar wrote:
> > > > On Wed, 2017-08-02 at 10:01 +0200, Jan Kara wrote:
> > > > > On Tue 01-08-17 16:24:30, Mimi Zohar wrote:
> > > > > > From: Christoph Hellwig <hc...@ls...>
> > > > > > 
> > > > > > Add a new ->integrity_read file operation to read data for integrity
> > > > > > hash collection.  This is defined to be equivalent to ->read_iter,
> > > > > > except that it will be called with the i_rwsem held exclusively.
> > > > > > 
> > > > > > Signed-off-by: Christoph Hellwig <hc...@ls...>
> > > > > > Cc: Matthew Garrett <mat...@ne...>
> > > > > > Cc: Jan Kara <ja...@su...>
> > > > > > Cc: "Theodore Ts'o" <ty...@mi...>
> > > > > > Cc: Andreas Dilger <adi...@di...>
> > > > > > Cc: Jaegeuk Kim <ja...@ke...>
> > > > > > Cc: Chao Yu <yu...@hu...>
> > > > > > Cc: Steven Whitehouse <swh...@re...>
> > > > > > Cc: Bob Peterson <rpe...@re...>
> > > > > > Cc: David Woodhouse <dw...@in...>
> > > > > > Cc: Dave Kleikamp <sh...@ke...>
> > > > > > Cc: Ryusuke Konishi <kon...@la...>
> > > > > > Cc: Mark Fasheh <mf...@ve...>
> > > > > > Cc: Joel Becker <jl...@ev...>
> > > > > > Cc: Richard Weinberger <ri...@no...>
> > > > > > Cc: "Darrick J. Wong" <dar...@or...>
> > > > > > Cc: Hugh Dickins <hu...@go...>
> > > > > > Cc: Chris Mason <cl...@fb...>
> > > > > > Signed-off-by: Mimi Zohar <zo...@li...>
> > > > > 
> > > > > ...
> > > > > 
> > > > > > +static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb,
> > > > > > +					     struct iov_iter *to)
> > > > > > +{
> > > > > > +	struct inode *inode = file_inode(iocb->ki_filp);
> > > > > > +	int o_direct = iocb->ki_flags & IOCB_DIRECT;
> > > > > > +
> > > > > > +	lockdep_assert_held(&inode->i_rwsem);
> > > > > > +	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
> > > > > > +		return -EIO;
> > > > > > +
> > > > > > +	if (!iov_iter_count(to))
> > > > > > +		return 0; /* skip atime */
> > > > > > +
> > > > > > +#ifdef CONFIG_FS_DAX
> > > > > > +	if (IS_DAX(inode))
> > > > > > +		return dax_iomap_rw(iocb, to, &ext4_iomap_ops);
> > > > > > +#endif
> > > > > > +	if (o_direct)
> > > > > > +		return -EINVAL;
> > > > > > +	return generic_file_read_iter(iocb, to);
> > > > > > +}
> > > > > 
> > > > > I have noticed this o_direct check - why is it only in ext4 and shouldn't
> > > > > rather higher layers make sure IOCB_DIRECT iocbs cannot reach
> > > > > .integrity_read() methods?
> > > >  
> > > > This failure happens when opening a file with O_DIRECT on a block
> > > > device that does not support dax (eg. loop). xfs makes it to here too,
> > > > but the call to generic_file_read_iter() fails properly with -EINVAL. 
> > > > (Only tested on those filesystems included that support dax (eg. ext2,
> > > > ext4, and xfs).)
> > > 
> > > Well, yes, ext2 and ext4 will silently fall back to buffered read (as do
> > > pretty much all filesystems except for XFS). However I fail to see why IMA
> > > should care (which is probably due to my lack of knowledge about IMA).
> > > Is O_DIRECT somehow excepted from IMA? But then why it is not for DAX?
> > 
> > Thank you for the explanation!  (I was confused about the relationship
> > between O_DIRECT and DAX.)  You're correct.  IMA does not support
> > O_DIRECT in the buffered case for two reasons, locking and updating
> > the file hash, which are described in commit f9b2a735bddd "ima: audit
> > log files opened with O_DIRECT flag".  After reverting this commit,
> > the O_DIRECT check is needed before calling generic_file_read_iter().
> 
> Thanks for the pointer. This cleaned up the issue for me.

Great!

> >  Most likely the same would need to be done for other filesystems that
> > support O_DIRECT.  Probably a generic_integrity_file_read_iter()
> > should be defined.
> 
> Yeah, then please define some common helper that takes care of refusing
> direct IO - IMO you should check this even before calling into
> ->integrity_read helper.

Agreed.

> > For DAX, unlike do_blockdev_direct_IO() which takes the lock, reading
> > the file with O_DIRECT is fine, as dax_iomap_rw() only checks that the
> > lock has been taken.  Assuming the file system is mounted with
> > i_version, the file hash is updated properly.
> 
> Yes, for DAX direct IO is basically no different but frankly I would just
> refuse O_DIRECT on DAX inodes as well just for the consistency sake.

Ok.  So I shouldn't revert the original commit, which fails the
O_DIRECT open for either the buffered read or DAX.  I'll just move the
code to a bit later, so that the failure is added to the measurement
list.

The original commit returned -EACCES.  On xfs, the open for direct IO
buffer read fails with -EINVAL.  Do you have a preference IMA should
return?

thanks!

Mimi

Re: [Linux-ima-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

[Nayna <na...@li...>]

On 08/07/2017 05:22 PM, Peter Huewe wrote:
>
>
> Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain <na...@li...>:
>> The TPM burstcount status indicates the number of bytes that can
>> be sent to the TPM without causing bus wait states.  Effectively,
>> it is the number of empty bytes in the command FIFO. Further,
>> some TPMs have a static burstcount, when the value remains zero
>> until the entire FIFO is empty.
>>
>> This patch ignores burstcount, permitting wait states, and thus
>> writes the command as fast as the TPM can accept the bytes.
>> The performance of a 34 byte extend on a TPM 1.2 improved from
>> 52 msec to 11 msec.
>>
>> Suggested-by: Ken Goldman <kg...@li...> in
>> conjunction with the TPM Device Driver work group.
>> Signed-off-by: Nayna Jain <na...@li...>
>> Acked-by: Mimi Zohar <zo...@li...>
>
> Are you sure this is a good idea?
> On lpc systems this more or less stalls the bus, including keyboard/mouse (if connected via superio lpc).

Thanks Peter for quick response.

I actually meant to post this patch as RFC. Sorry, missed that.
It was meant to be a starting place for the discussion related to 
burst_count.

>
> On which systems have you tested this?
> Spi/Lpc? Architecture?

Tested it with LPC on x86.

>
> This might not be noticable for small transfers, but think about much larger transfers....

I did the following testing:

* Ran a script with 1000 extends. This was to test continuous extends
which are generally in large numbers when IMA is enabled.

* Ran a command to ask TPM to hash big size file like 1MB. This was to
test the long command.

In both of the above cases, I didn't face any tpm specific errors.

Is there any test-script or test-cases which I can try to test the
scenario(stalling the bus, including keyboard/mouse) with the patch ?

Thanks & Regards,
    - Nayna


>
> Imho: NACK from my side.
>
> Thanks,
> Peter
>
>> ---
>> drivers/char/tpm/tpm_tis_core.c | 45
>> ++---------------------------------------
>> 1 file changed, 2 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c
>> b/drivers/char/tpm/tpm_tis_core.c
>> index b617b2eeb080..478cbc0f61c3 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -255,9 +255,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8
>> *buf, size_t count)
>> static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t
>> len)
>> {
>> 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> -	int rc, status, burstcnt;
>> -	size_t count = 0;
>> -	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>> +	int rc, status;
>>
>> 	status = tpm_tis_status(chip);
>> 	if ((status & TPM_STS_COMMAND_READY) == 0) {
>> @@ -270,49 +268,10 @@ static int tpm_tis_send_data(struct tpm_chip
>> *chip, u8 *buf, size_t len)
>> 		}
>> 	}
>>
>> -	while (count < len - 1) {
>> -		burstcnt = get_burstcount(chip);
>> -		if (burstcnt < 0) {
>> -			dev_err(&chip->dev, "Unable to read burstcount\n");
>> -			rc = burstcnt;
>> -			goto out_err;
>> -		}
>> -		burstcnt = min_t(int, burstcnt, len - count - 1);
>> -		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>> -					 burstcnt, buf + count);
>> -		if (rc < 0)
>> -			goto out_err;
>> -
>> -		count += burstcnt;
>> -
>> -		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>> -					&priv->int_queue, false) < 0) {
>> -			rc = -ETIME;
>> -			goto out_err;
>> -		}
>> -		status = tpm_tis_status(chip);
>> -		if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
>> -			rc = -EIO;
>> -			goto out_err;
>> -		}
>> -	}
>> -
>> -	/* write last byte */
>> -	rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]);
>> +	rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), len,
>> buf);
>> 	if (rc < 0)
>> 		goto out_err;
>>
>> -	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>> -				&priv->int_queue, false) < 0) {
>> -		rc = -ETIME;
>> -		goto out_err;
>> -	}
>> -	status = tpm_tis_status(chip);
>> -	if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) {
>> -		rc = -EIO;
>> -		goto out_err;
>> -	}
>> -
>> 	return 0;
>>
>> out_err:
>

Re: [Linux-ima-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

[Peter H. <pet...@gm...>]

Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain <na...@li...>:
>The TPM burstcount status indicates the number of bytes that can
>be sent to the TPM without causing bus wait states.  Effectively,
>it is the number of empty bytes in the command FIFO. Further,
>some TPMs have a static burstcount, when the value remains zero
>until the entire FIFO is empty.
>
>This patch ignores burstcount, permitting wait states, and thus
>writes the command as fast as the TPM can accept the bytes.
>The performance of a 34 byte extend on a TPM 1.2 improved from
>52 msec to 11 msec.
>
>Suggested-by: Ken Goldman <kg...@li...> in
>conjunction with the TPM Device Driver work group.
>Signed-off-by: Nayna Jain <na...@li...>
>Acked-by: Mimi Zohar <zo...@li...>

Are you sure this is a good idea?
On lpc systems this more or less stalls the bus, including keyboard/mouse (if connected via superio lpc).

On which systems have you tested this?
Spi/Lpc? Architecture?

This might not be noticable for small transfers, but think about much larger transfers....

Imho: NACK from my side.

Thanks,
Peter

>---
>drivers/char/tpm/tpm_tis_core.c | 45
>++---------------------------------------
> 1 file changed, 2 insertions(+), 43 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm_tis_core.c
>b/drivers/char/tpm/tpm_tis_core.c
>index b617b2eeb080..478cbc0f61c3 100644
>--- a/drivers/char/tpm/tpm_tis_core.c
>+++ b/drivers/char/tpm/tpm_tis_core.c
>@@ -255,9 +255,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8
>*buf, size_t count)
>static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t
>len)
> {
> 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>-	int rc, status, burstcnt;
>-	size_t count = 0;
>-	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>+	int rc, status;
> 
> 	status = tpm_tis_status(chip);
> 	if ((status & TPM_STS_COMMAND_READY) == 0) {
>@@ -270,49 +268,10 @@ static int tpm_tis_send_data(struct tpm_chip
>*chip, u8 *buf, size_t len)
> 		}
> 	}
> 
>-	while (count < len - 1) {
>-		burstcnt = get_burstcount(chip);
>-		if (burstcnt < 0) {
>-			dev_err(&chip->dev, "Unable to read burstcount\n");
>-			rc = burstcnt;
>-			goto out_err;
>-		}
>-		burstcnt = min_t(int, burstcnt, len - count - 1);
>-		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>-					 burstcnt, buf + count);
>-		if (rc < 0)
>-			goto out_err;
>-
>-		count += burstcnt;
>-
>-		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>-					&priv->int_queue, false) < 0) {
>-			rc = -ETIME;
>-			goto out_err;
>-		}
>-		status = tpm_tis_status(chip);
>-		if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
>-			rc = -EIO;
>-			goto out_err;
>-		}
>-	}
>-
>-	/* write last byte */
>-	rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]);
>+	rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), len,
>buf);
> 	if (rc < 0)
> 		goto out_err;
> 
>-	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>-				&priv->int_queue, false) < 0) {
>-		rc = -ETIME;
>-		goto out_err;
>-	}
>-	status = tpm_tis_status(chip);
>-	if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) {
>-		rc = -EIO;
>-		goto out_err;
>-	}
>-
> 	return 0;
> 
> out_err:

-- 
Sent from my mobile

[Linux-ima-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

[Nayna J. <na...@li...>]

The TPM burstcount status indicates the number of bytes that can
be sent to the TPM without causing bus wait states.  Effectively,
it is the number of empty bytes in the command FIFO. Further,
some TPMs have a static burstcount, when the value remains zero
until the entire FIFO is empty.

This patch ignores burstcount, permitting wait states, and thus
writes the command as fast as the TPM can accept the bytes.
The performance of a 34 byte extend on a TPM 1.2 improved from
52 msec to 11 msec.

Suggested-by: Ken Goldman <kg...@li...> in
conjunction with the TPM Device Driver work group.
Signed-off-by: Nayna Jain <na...@li...>
Acked-by: Mimi Zohar <zo...@li...>
---
 drivers/char/tpm/tpm_tis_core.c | 45 ++---------------------------------------
 1 file changed, 2 insertions(+), 43 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index b617b2eeb080..478cbc0f61c3 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -255,9 +255,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
-	int rc, status, burstcnt;
-	size_t count = 0;
-	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
+	int rc, status;
 
 	status = tpm_tis_status(chip);
 	if ((status & TPM_STS_COMMAND_READY) == 0) {
@@ -270,49 +268,10 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 		}
 	}
 
-	while (count < len - 1) {
-		burstcnt = get_burstcount(chip);
-		if (burstcnt < 0) {
-			dev_err(&chip->dev, "Unable to read burstcount\n");
-			rc = burstcnt;
-			goto out_err;
-		}
-		burstcnt = min_t(int, burstcnt, len - count - 1);
-		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
-					 burstcnt, buf + count);
-		if (rc < 0)
-			goto out_err;
-
-		count += burstcnt;
-
-		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
-					&priv->int_queue, false) < 0) {
-			rc = -ETIME;
-			goto out_err;
-		}
-		status = tpm_tis_status(chip);
-		if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
-			rc = -EIO;
-			goto out_err;
-		}
-	}
-
-	/* write last byte */
-	rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]);
+	rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), len, buf);
 	if (rc < 0)
 		goto out_err;
 
-	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
-				&priv->int_queue, false) < 0) {
-		rc = -ETIME;
-		goto out_err;
-	}
-	status = tpm_tis_status(chip);
-	if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) {
-		rc = -EIO;
-		goto out_err;
-	}
-
 	return 0;
 
 out_err:
-- 
2.13.3

Re: [Linux-ima-devel] [PATCH v4 2/5] ima: use fs method to read integrity data [updated]

[Jan K. <ja...@su...>]

On Fri 04-08-17 17:07:11, Mimi Zohar wrote:
> On Thu, 2017-08-03 at 12:56 +0200, Jan Kara wrote:
> > On Wed 02-08-17 13:11:52, Mimi Zohar wrote:
> > > On Wed, 2017-08-02 at 10:01 +0200, Jan Kara wrote:
> > > > On Tue 01-08-17 16:24:30, Mimi Zohar wrote:
> > > > > From: Christoph Hellwig <hc...@ls...>
> > > > > 
> > > > > Add a new ->integrity_read file operation to read data for integrity
> > > > > hash collection.  This is defined to be equivalent to ->read_iter,
> > > > > except that it will be called with the i_rwsem held exclusively.
> > > > > 
> > > > > Signed-off-by: Christoph Hellwig <hc...@ls...>
> > > > > Cc: Matthew Garrett <mat...@ne...>
> > > > > Cc: Jan Kara <ja...@su...>
> > > > > Cc: "Theodore Ts'o" <ty...@mi...>
> > > > > Cc: Andreas Dilger <adi...@di...>
> > > > > Cc: Jaegeuk Kim <ja...@ke...>
> > > > > Cc: Chao Yu <yu...@hu...>
> > > > > Cc: Steven Whitehouse <swh...@re...>
> > > > > Cc: Bob Peterson <rpe...@re...>
> > > > > Cc: David Woodhouse <dw...@in...>
> > > > > Cc: Dave Kleikamp <sh...@ke...>
> > > > > Cc: Ryusuke Konishi <kon...@la...>
> > > > > Cc: Mark Fasheh <mf...@ve...>
> > > > > Cc: Joel Becker <jl...@ev...>
> > > > > Cc: Richard Weinberger <ri...@no...>
> > > > > Cc: "Darrick J. Wong" <dar...@or...>
> > > > > Cc: Hugh Dickins <hu...@go...>
> > > > > Cc: Chris Mason <cl...@fb...>
> > > > > Signed-off-by: Mimi Zohar <zo...@li...>
> > > > 
> > > > ...
> > > > 
> > > > > +static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb,
> > > > > +					     struct iov_iter *to)
> > > > > +{
> > > > > +	struct inode *inode = file_inode(iocb->ki_filp);
> > > > > +	int o_direct = iocb->ki_flags & IOCB_DIRECT;
> > > > > +
> > > > > +	lockdep_assert_held(&inode->i_rwsem);
> > > > > +	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
> > > > > +		return -EIO;
> > > > > +
> > > > > +	if (!iov_iter_count(to))
> > > > > +		return 0; /* skip atime */
> > > > > +
> > > > > +#ifdef CONFIG_FS_DAX
> > > > > +	if (IS_DAX(inode))
> > > > > +		return dax_iomap_rw(iocb, to, &ext4_iomap_ops);
> > > > > +#endif
> > > > > +	if (o_direct)
> > > > > +		return -EINVAL;
> > > > > +	return generic_file_read_iter(iocb, to);
> > > > > +}
> > > > 
> > > > I have noticed this o_direct check - why is it only in ext4 and shouldn't
> > > > rather higher layers make sure IOCB_DIRECT iocbs cannot reach
> > > > .integrity_read() methods?
> > >  
> > > This failure happens when opening a file with O_DIRECT on a block
> > > device that does not support dax (eg. loop). xfs makes it to here too,
> > > but the call to generic_file_read_iter() fails properly with -EINVAL. 
> > > (Only tested on those filesystems included that support dax (eg. ext2,
> > > ext4, and xfs).)
> > 
> > Well, yes, ext2 and ext4 will silently fall back to buffered read (as do
> > pretty much all filesystems except for XFS). However I fail to see why IMA
> > should care (which is probably due to my lack of knowledge about IMA).
> > Is O_DIRECT somehow excepted from IMA? But then why it is not for DAX?
> 
> Thank you for the explanation!  (I was confused about the relationship
> between O_DIRECT and DAX.)  You're correct.  IMA does not support
> O_DIRECT in the buffered case for two reasons, locking and updating
> the file hash, which are described in commit f9b2a735bddd "ima: audit
> log files opened with O_DIRECT flag".  After reverting this commit,
> the O_DIRECT check is needed before calling generic_file_read_iter().

Thanks for the pointer. This cleaned up the issue for me.

>  Most likely the same would need to be done for other filesystems that
> support O_DIRECT.  Probably a generic_integrity_file_read_iter()
> should be defined.

Yeah, then please define some common helper that takes care of refusing
direct IO - IMO you should check this even before calling into
->integrity_read helper.

> For DAX, unlike do_blockdev_direct_IO() which takes the lock, reading
> the file with O_DIRECT is fine, as dax_iomap_rw() only checks that the
> lock has been taken.  Assuming the file system is mounted with
> i_version, the file hash is updated properly.

Yes, for DAX direct IO is basically no different but frankly I would just
refuse O_DIRECT on DAX inodes as well just for the consistency sake.

								Honza
-- 
Jan Kara <ja...@su...>
SUSE Labs, CR

[Linux-ima-devel] [PATCH v4 7/7] ima: Support module-style appended signatures for appraisal

[Thiago J. B. <bau...@li...>]

This patch introduces the modsig keyword to the IMA policy syntax to
specify that a given hook should expect the file to have the IMA signature
appended to it. Here is how it can be used in a rule:

appraise func=KEXEC_KERNEL_CHECK appraise_type=modsig|imasig

With this rule, IMA will accept either an appended signature or a signature
stored in the extended attribute. In that case, it will first check whether
there is an appended signature, and if not it will read it from the
extended attribute.

The format of the appended signature is the same used for signed kernel
modules. This means that the file can be signed with the scripts/sign-file
tool, with a command line such as this:

$ sign-file sha256 privkey_ima.pem x509_ima.der vmlinux

This code only works for files that are hashed from a memory buffer, not
for files that are read from disk at the time of hash calculation. In other
words, only hooks that use kernel_read_file can support appended
signatures. This means that only FIRMWARE_CHECK, KEXEC_KERNEL_CHECK,
KEXEC_INITRAMFS_CHECK and POLICY_CHECK can be supported.

This feature warrants a separate config option because enabling it brings
in many other config options.

Signed-off-by: Thiago Jung Bauermann <bau...@li...>
---
 security/integrity/ima/Kconfig            |  13 +++
 security/integrity/ima/Makefile           |   1 +
 security/integrity/ima/ima.h              |  70 +++++++++++-
 security/integrity/ima/ima_appraise.c     | 178 +++++++++++++++++++++++++-----
 security/integrity/ima/ima_main.c         |   7 +-
 security/integrity/ima/ima_modsig.c       | 178 ++++++++++++++++++++++++++++++
 security/integrity/ima/ima_policy.c       |  26 +++--
 security/integrity/ima/ima_template_lib.c |  14 ++-
 security/integrity/integrity.h            |   4 +-
 9 files changed, 443 insertions(+), 48 deletions(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 35ef69312811..55f734a6124b 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -163,6 +163,19 @@ config IMA_APPRAISE_BOOTPARAM
 	  This option enables the different "ima_appraise=" modes
 	  (eg. fix, log) from the boot command line.
 
+config IMA_APPRAISE_MODSIG
+	bool "Support module-style signatures for appraisal"
+	depends on IMA_APPRAISE
+	depends on INTEGRITY_ASYMMETRIC_KEYS
+	select PKCS7_MESSAGE_PARSER
+	select MODULE_SIG_FORMAT
+	default n
+	help
+	   Adds support for signatures appended to files. The format of the
+	   appended signature is the same used for signed kernel modules.
+	   The modsig keyword can be used in the IMA policy to allow a hook
+	   to accept such signatures.
+
 config IMA_TRUSTED_KEYRING
 	bool "Require all keys on the .ima keyring be signed (deprecated)"
 	depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 29f198bde02b..c72026acecc3 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -8,5 +8,6 @@ obj-$(CONFIG_IMA) += ima.o
 ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
 	 ima_policy.o ima_template.o ima_template_lib.o
 ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
+ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
 ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
 obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d52b487ad259..5492af2cd7c7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -190,6 +190,8 @@ enum ima_hooks {
 	__ima_hooks(__ima_hook_enumify)
 };
 
+extern const char *const func_tokens[];
+
 /* LIM API function definitions */
 int ima_get_action(struct inode *inode, int mask,
 		   enum ima_hooks func, int *pcr);
@@ -236,9 +238,10 @@ int ima_policy_show(struct seq_file *m, void *v);
 #ifdef CONFIG_IMA_APPRAISE
 int ima_appraise_measurement(enum ima_hooks func,
 			     struct integrity_iint_cache *iint,
-			     struct file *file, const unsigned char *filename,
-			     struct evm_ima_xattr_data *xattr_value,
-			     int xattr_len, int opened);
+			     struct file *file, const void *buf, loff_t size,
+			     const unsigned char *filename,
+			     struct evm_ima_xattr_data **xattr_value,
+			     int *xattr_len, int opened);
 int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func);
 void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
 enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
@@ -248,13 +251,28 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 int ima_read_xattr(struct dentry *dentry,
 		   struct evm_ima_xattr_data **xattr_value);
 
+#ifdef CONFIG_IMA_APPRAISE_MODSIG
+bool ima_hook_supports_modsig(enum ima_hooks func);
+int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
+		    struct evm_ima_xattr_data **xattr_value,
+		    int *xattr_len);
+int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
+			void const **hash, u8 *len);
+int ima_modsig_serialize_data(struct evm_ima_xattr_data *hdr,
+			      struct evm_ima_xattr_data **data, int *data_len);
+int ima_modsig_verify(const unsigned int keyring_id,
+		      struct evm_ima_xattr_data *hdr);
+void ima_free_xattr_data(struct evm_ima_xattr_data *hdr);
+#endif
+
 #else
 static inline int ima_appraise_measurement(enum ima_hooks func,
 					   struct integrity_iint_cache *iint,
-					   struct file *file,
+					   struct file *file, const void *buf,
+					   loff_t size,
 					   const unsigned char *filename,
-					   struct evm_ima_xattr_data *xattr_value,
-					   int xattr_len, int opened)
+					   struct evm_ima_xattr_data **xattr_value,
+					   int *xattr_len, int opened)
 {
 	return INTEGRITY_UNKNOWN;
 }
@@ -291,6 +309,46 @@ static inline int ima_read_xattr(struct dentry *dentry,
 
 #endif /* CONFIG_IMA_APPRAISE */
 
+#ifndef CONFIG_IMA_APPRAISE_MODSIG
+static inline bool ima_hook_supports_modsig(enum ima_hooks func)
+{
+	return false;
+}
+
+static inline int ima_read_modsig(enum ima_hooks func, const void *buf,
+				  loff_t buf_len,
+				  struct evm_ima_xattr_data **xattr_value,
+				  int *xattr_len)
+{
+	return -ENOTSUPP;
+}
+
+static inline int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr,
+				      enum hash_algo *algo, void const **hash,
+				      u8 *len)
+{
+	return -ENOTSUPP;
+}
+
+static inline int ima_modsig_serialize_data(struct evm_ima_xattr_data *hdr,
+					    struct evm_ima_xattr_data **data,
+					    int *data_len)
+{
+	return -ENOTSUPP;
+}
+
+static inline int ima_modsig_verify(const unsigned int keyring_id,
+				    struct evm_ima_xattr_data *hdr)
+{
+	return -ENOTSUPP;
+}
+
+static inline void ima_free_xattr_data(struct evm_ima_xattr_data *hdr)
+{
+	kfree(hdr);
+}
+#endif
+
 /* LSM based policy rules require audit */
 #ifdef CONFIG_IMA_LSM_RULES
 
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 87d2b601cf8e..5a244ebc61d9 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -190,6 +190,64 @@ int ima_read_xattr(struct dentry *dentry,
 	return ret;
 }
 
+static void process_xattr_error(int rc, struct integrity_iint_cache *iint,
+				int opened, char const **cause,
+				enum integrity_status *status)
+{
+	if (rc && rc != -ENODATA)
+		return;
+
+	*cause = iint->flags & IMA_DIGSIG_REQUIRED ?
+		"IMA-signature-required" : "missing-hash";
+	*status = INTEGRITY_NOLABEL;
+
+	if (opened & FILE_CREATED)
+		iint->flags |= IMA_NEW_FILE;
+
+	if ((iint->flags & IMA_NEW_FILE) &&
+	    !(iint->flags & IMA_DIGSIG_REQUIRED))
+		*status = INTEGRITY_PASS;
+}
+
+static int appraise_modsig(struct integrity_iint_cache *iint,
+			   struct evm_ima_xattr_data *xattr_value,
+			   int xattr_len)
+{
+	enum hash_algo algo;
+	const void *digest;
+	void *buf;
+	int rc, len;
+	u8 dig_len;
+
+	rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, xattr_value);
+	if (rc)
+		return rc;
+
+	/*
+	 * The signature is good. Now let's put the sig hash
+	 * into the iint cache so that it gets stored in the
+	 * measurement list.
+	 */
+
+	rc = ima_get_modsig_hash(xattr_value, &algo, &digest, &dig_len);
+	if (rc)
+		return rc;
+
+	len = sizeof(iint->ima_hash) + dig_len;
+	buf = krealloc(iint->ima_hash, len, GFP_NOFS);
+	if (!buf)
+		return -ENOMEM;
+
+	iint->ima_hash = buf;
+	iint->flags |= IMA_DIGSIG;
+	iint->ima_hash->algo = algo;
+	iint->ima_hash->length = dig_len;
+
+	memcpy(iint->ima_hash->digest, digest, dig_len);
+
+	return 0;
+}
+
 /*
  * ima_appraise_measurement - appraise file measurement
  *
@@ -200,44 +258,55 @@ int ima_read_xattr(struct dentry *dentry,
  */
 int ima_appraise_measurement(enum ima_hooks func,
 			     struct integrity_iint_cache *iint,
-			     struct file *file, const unsigned char *filename,
-			     struct evm_ima_xattr_data *xattr_value,
-			     int xattr_len, int opened)
+			     struct file *file, const void *buf, loff_t size,
+			     const unsigned char *filename,
+			     struct evm_ima_xattr_data **xattr_value_,
+			     int *xattr_len_, int opened)
 {
 	static const char op[] = "appraise_data";
-	char *cause = "unknown";
+	const char *cause = "unknown";
 	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = d_backing_inode(dentry);
 	enum integrity_status status = INTEGRITY_UNKNOWN;
-	int rc = xattr_len, hash_start = 0;
+	struct evm_ima_xattr_data *xattr_value = *xattr_value_;
+	int xattr_len = *xattr_len_, rc = xattr_len, hash_start = 0;
+	bool appraising_modsig = false;
+
+	if (iint->flags & IMA_MODSIG_ALLOWED &&
+	    !ima_read_modsig(func, buf, size, &xattr_value, &xattr_len)) {
+		appraising_modsig = true;
+		rc = xattr_len;
+	}
 
-	if (!(inode->i_opflags & IOP_XATTR))
+	/* If not appraising a modsig, we need an xattr. */
+	if (!appraising_modsig && !(inode->i_opflags & IOP_XATTR))
 		return INTEGRITY_UNKNOWN;
 
 	if (rc <= 0) {
-		if (rc && rc != -ENODATA)
-			goto out;
-
-		cause = iint->flags & IMA_DIGSIG_REQUIRED ?
-				"IMA-signature-required" : "missing-hash";
-		status = INTEGRITY_NOLABEL;
-		if (opened & FILE_CREATED)
-			iint->flags |= IMA_NEW_FILE;
-		if ((iint->flags & IMA_NEW_FILE) &&
-		    !(iint->flags & IMA_DIGSIG_REQUIRED))
-			status = INTEGRITY_PASS;
+		process_xattr_error(rc, iint, opened, &cause, &status);
 		goto out;
 	}
 
-	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
-	if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
-		if ((status == INTEGRITY_NOLABEL)
-		    || (status == INTEGRITY_NOXATTRS))
-			cause = "missing-HMAC";
-		else if (status == INTEGRITY_FAIL)
-			cause = "invalid-HMAC";
+	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, NULL, 0, iint);
+	switch (status) {
+	case INTEGRITY_PASS:
+	case INTEGRITY_UNKNOWN:
+		break;
+	case INTEGRITY_NOXATTRS:
+		/* No EVM protected xattrs. */
+		if (appraising_modsig)
+			break;
+	case INTEGRITY_NOLABEL:
+		/* No security.evm xattr. */
+		cause = "missing-HMAC";
+		goto out;
+	case INTEGRITY_FAIL:
+		/* Invalid HMAC/signature. */
+		cause = "invalid-HMAC";
 		goto out;
 	}
+
+ retry:
 	switch (xattr_value->type) {
 	case IMA_XATTR_DIGEST_NG:
 		/* first byte contains algorithm id */
@@ -281,6 +350,54 @@ int ima_appraise_measurement(enum ima_hooks func,
 			status = INTEGRITY_PASS;
 		}
 		break;
+	case IMA_MODSIG:
+		/*
+		 * To avoid being tricked into an infinite loop, we don't allow
+		 * a modsig stored in the xattr.
+		 */
+		if (!appraising_modsig) {
+			status = INTEGRITY_UNKNOWN;
+			cause = "unknown-ima-data";
+			break;
+		}
+
+		rc = appraise_modsig(iint, xattr_value, xattr_len);
+		if (!rc) {
+			kfree(*xattr_value_);
+			*xattr_value_ = xattr_value;
+			*xattr_len_ = xattr_len;
+
+			status = INTEGRITY_PASS;
+			break;
+		}
+
+		ima_free_xattr_data(xattr_value);
+
+		/*
+		 * The appended signature failed verification. Let's see if
+		 * there's an extended attribute to fall back to.
+		 */
+		if (inode->i_opflags & IOP_XATTR && *xattr_len_ != 0) {
+			xattr_value = *xattr_value_;
+			xattr_len = rc = *xattr_len_;
+			appraising_modsig = false;
+
+			if (rc <= 0) {
+				process_xattr_error(rc, iint, opened, &cause,
+						    &status);
+				goto out;
+			}
+
+			goto retry;
+		}
+
+		if (rc == -EOPNOTSUPP)
+			status = INTEGRITY_UNKNOWN;
+		else if (rc) {
+			cause = "invalid-signature";
+			status = INTEGRITY_FAIL;
+		}
+		break;
 	default:
 		status = INTEGRITY_UNKNOWN;
 		cause = "unknown-ima-data";
@@ -291,13 +408,15 @@ int ima_appraise_measurement(enum ima_hooks func,
 	if (status != INTEGRITY_PASS) {
 		if ((ima_appraise & IMA_APPRAISE_FIX) &&
 		    (!xattr_value ||
-		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
+		     (xattr_value->type != EVM_IMA_XATTR_DIGSIG &&
+		      xattr_value->type != IMA_MODSIG))) {
 			if (!ima_fix_xattr(dentry, iint))
 				status = INTEGRITY_PASS;
 		} else if ((inode->i_size == 0) &&
 			   (iint->flags & IMA_NEW_FILE) &&
 			   (xattr_value &&
-			    xattr_value->type == EVM_IMA_XATTR_DIGSIG)) {
+			    (xattr_value->type == EVM_IMA_XATTR_DIGSIG ||
+			     xattr_value->type == IMA_MODSIG))) {
 			status = INTEGRITY_PASS;
 		}
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
@@ -398,6 +517,7 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		       const void *xattr_value, size_t xattr_value_len)
 {
 	const struct evm_ima_xattr_data *xvalue = xattr_value;
+	bool digsig;
 	int result;
 
 	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
@@ -405,8 +525,10 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 	if (result == 1) {
 		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
 			return -EINVAL;
-		ima_reset_appraise_flags(d_backing_inode(dentry),
-			 (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0);
+
+		digsig = xvalue->type == EVM_IMA_XATTR_DIGSIG ||
+				xvalue->type == IMA_MODSIG;
+		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
 		result = 0;
 	}
 	return result;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 0b4845e7248d..93fa257c71a7 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -245,8 +245,9 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
 
 	if (action & IMA_APPRAISE_SUBMASK)
-		rc = ima_appraise_measurement(func, iint, file, pathname,
-					      xattr_value, xattr_len, opened);
+		rc = ima_appraise_measurement(func, iint, file, buf, size,
+					      pathname, &xattr_value,
+					      &xattr_len, opened);
 	if (action & IMA_MEASURE)
 		ima_store_measurement(iint, file, pathname,
 				      xattr_value, xattr_len, pcr);
@@ -257,7 +258,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG) &&
 	     !(iint->flags & IMA_NEW_FILE))
 		rc = -EACCES;
-	kfree(xattr_value);
+	ima_free_xattr_data(xattr_value);
 out_free:
 	if (pathbuf)
 		__putname(pathbuf);
diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
new file mode 100644
index 000000000000..7983edf84511
--- /dev/null
+++ b/security/integrity/ima/ima_modsig.c
@@ -0,0 +1,178 @@
+/*
+ * IMA support for appraising module-style appended signatures.
+ *
+ * Copyright (C) 2017  IBM Corporation
+ *
+ * Author:
+ * Thiago Jung Bauermann <bau...@li...>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation (version 2 of the License).
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/types.h>
+#include <linux/module_signature.h>
+#include <keys/asymmetric-type.h>
+#include <crypto/pkcs7.h>
+
+#include "ima.h"
+
+struct modsig_hdr {
+	uint8_t type;		/* Should be IMA_MODSIG. */
+	const void *data;	/* Pointer to data covered by pkcs7_msg. */
+	size_t data_len;
+	struct pkcs7_message *pkcs7_msg;
+	int raw_pkcs7_len;
+
+	/* This will be in the measurement list if required by the template. */
+	struct evm_ima_xattr_data raw_pkcs7;
+};
+
+/**
+ * ima_hook_supports_modsig - can the policy allow modsig for this hook?
+ *
+ * modsig is only supported by hooks using ima_post_read_file, because only they
+ * preload the contents of the file in a buffer. FILE_CHECK does that in some
+ * cases, but not when reached from vfs_open. POLICY_CHECK can support it, but
+ * it's not useful in practice because it's a text file so deny.
+ */
+bool ima_hook_supports_modsig(enum ima_hooks func)
+{
+	switch (func) {
+	case FIRMWARE_CHECK:
+	case KEXEC_KERNEL_CHECK:
+	case KEXEC_INITRAMFS_CHECK:
+		return true;
+	default:
+		return false;
+	}
+}
+
+int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
+		    struct evm_ima_xattr_data **xattr_value,
+		    int *xattr_len)
+{
+	const size_t marker_len = sizeof(MODULE_SIG_STRING) - 1;
+	const struct module_signature *sig;
+	struct modsig_hdr *hdr;
+	size_t sig_len;
+	const void *p;
+	int rc;
+
+	/*
+	 * Not supposed to happen. Hooks that support modsig are
+	 * whitelisted when parsing the policy using
+	 * ima_hooks_supports_modsig.
+	 */
+	if (!buf || !buf_len) {
+		WARN_ONCE(true, "%s doesn't support modsig\n",
+			  func_tokens[func]);
+		return -ENOENT;
+	} else if (buf_len <= marker_len + sizeof(*sig))
+		return -ENOENT;
+
+	p = buf + buf_len - marker_len;
+	if (memcmp(p, MODULE_SIG_STRING, marker_len))
+		return -ENOENT;
+
+	buf_len -= marker_len;
+	sig = (const struct module_signature *) (p - sizeof(*sig));
+
+	rc = validate_module_sig(sig, buf_len);
+	if (rc)
+		return rc;
+
+	sig_len = be32_to_cpu(sig->sig_len);
+	buf_len -= sig_len + sizeof(*sig);
+
+	hdr = kmalloc(sizeof(*hdr) + sig_len, GFP_KERNEL);
+	if (!hdr)
+		return -ENOMEM;
+
+	hdr->pkcs7_msg = pkcs7_parse_message(buf + buf_len, sig_len);
+	if (IS_ERR(hdr->pkcs7_msg)) {
+		rc = PTR_ERR(hdr->pkcs7_msg);
+		kfree(hdr);
+		return rc;
+	}
+
+	memcpy(hdr->raw_pkcs7.data, buf + buf_len, sig_len);
+	hdr->raw_pkcs7_len = sig_len + 1;
+	hdr->raw_pkcs7.type = IMA_MODSIG;
+
+	hdr->type = IMA_MODSIG;
+	hdr->data = buf;
+	hdr->data_len = buf_len;
+
+	*xattr_value = (typeof(*xattr_value)) hdr;
+	*xattr_len = sizeof(*hdr);
+
+	return 0;
+}
+
+int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
+			void const **hash, u8 *len)
+{
+	const struct public_key_signature *pks;
+	struct modsig_hdr *modsig = (typeof(modsig)) hdr;
+	int i;
+
+	pks = pkcs7_get_message_sig(modsig->pkcs7_msg);
+	if (!pks)
+		return -EBADMSG;
+
+	for (i = 0; i < HASH_ALGO__LAST; i++)
+		if (!strcmp(hash_algo_name[i], pks->hash_algo))
+			break;
+
+	*algo = i;
+	*hash = pks->digest;
+	*len = pks->digest_size;
+
+	return 0;
+}
+
+int ima_modsig_serialize_data(struct evm_ima_xattr_data *hdr,
+			      struct evm_ima_xattr_data **data, int *data_len)
+{
+	struct modsig_hdr *modsig = (struct modsig_hdr *) hdr;
+
+	*data = &modsig->raw_pkcs7;
+	*data_len = modsig->raw_pkcs7_len;
+
+	return 0;
+}
+
+int ima_modsig_verify(const unsigned int keyring_id,
+		      struct evm_ima_xattr_data *hdr)
+{
+	struct modsig_hdr *modsig = (struct modsig_hdr *) hdr;
+	struct key *trusted_keys = integrity_keyring_from_id(keyring_id);
+
+	if (IS_ERR(trusted_keys))
+		return -EINVAL;
+
+	return verify_pkcs7_message_sig(modsig->data, modsig->data_len,
+					modsig->pkcs7_msg, trusted_keys,
+					VERIFYING_MODULE_SIGNATURE, NULL, NULL);
+}
+
+void ima_free_xattr_data(struct evm_ima_xattr_data *hdr)
+{
+	if (!hdr)
+		return;
+
+	if (hdr->type == IMA_MODSIG) {
+		struct modsig_hdr *modsig = (struct modsig_hdr *) hdr;
+
+		pkcs7_free_message(modsig->pkcs7_msg);
+	}
+
+	kfree(hdr);
+}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 95209a5f8595..cb056d20e6e5 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -851,8 +851,12 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			}
 
 			ima_log_string(ab, "appraise_type", args[0].from);
-			if ((strcmp(args[0].from, "imasig")) == 0)
+			if (strcmp(args[0].from, "imasig") == 0)
 				entry->flags |= IMA_DIGSIG_REQUIRED;
+			else if (ima_hook_supports_modsig(entry->func) &&
+				 strcmp(args[0].from, "modsig|imasig") == 0)
+				entry->flags |= IMA_DIGSIG_REQUIRED
+						| IMA_MODSIG_ALLOWED;
 			else
 				result = -EINVAL;
 			break;
@@ -958,6 +962,12 @@ void ima_delete_rules(void)
 	}
 }
 
+#define __ima_hook_stringify(str)	(#str),
+
+const char *const func_tokens[] = {
+	__ima_hooks(__ima_hook_stringify)
+};
+
 #ifdef	CONFIG_IMA_READ_POLICY
 enum {
 	mask_exec = 0, mask_write, mask_read, mask_append
@@ -970,12 +980,6 @@ static const char *const mask_tokens[] = {
 	"MAY_APPEND"
 };
 
-#define __ima_hook_stringify(str)	(#str),
-
-static const char *const func_tokens[] = {
-	__ima_hooks(__ima_hook_stringify)
-};
-
 void *ima_policy_start(struct seq_file *m, loff_t *pos)
 {
 	loff_t l = *pos;
@@ -1138,8 +1142,12 @@ int ima_policy_show(struct seq_file *m, void *v)
 			}
 		}
 	}
-	if (entry->flags & IMA_DIGSIG_REQUIRED)
-		seq_puts(m, "appraise_type=imasig ");
+	if (entry->flags & IMA_DIGSIG_REQUIRED) {
+		if (entry->flags & IMA_MODSIG_ALLOWED)
+			seq_puts(m, "appraise_type=modsig|imasig ");
+		else
+			seq_puts(m, "appraise_type=imasig ");
+	}
 	if (entry->flags & IMA_PERMIT_DIRECTIO)
 		seq_puts(m, "permit_directio ");
 	rcu_read_unlock();
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 28af43f63572..8c7fa52604a5 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -383,9 +383,21 @@ int ima_eventsig_init(struct ima_event_data *event_data,
 	int xattr_len = event_data->xattr_len;
 	int rc = 0;
 
-	if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
+	if (!xattr_value || (xattr_value->type != EVM_IMA_XATTR_DIGSIG &&
+			     xattr_value->type != IMA_MODSIG))
 		goto out;
 
+	/*
+	 * The xattr_value for IMA_MODSIG is a runtime structure containing
+	 * pointers. Get its raw data instead.
+	 */
+	if (xattr_value->type == IMA_MODSIG) {
+		rc = ima_modsig_serialize_data(xattr_value, &xattr_value,
+					       &xattr_len);
+		if (rc)
+			goto out;
+	}
+
 	rc = ima_write_template_field_data(xattr_value, xattr_len, fmt,
 					   field_data);
 out:
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 1f8f1a31d487..0b8b9efa0d2e 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -28,11 +28,12 @@
 
 /* iint cache flags */
 #define IMA_ACTION_FLAGS	0xff000000
-#define IMA_ACTION_RULE_FLAGS	0x06000000
+#define IMA_ACTION_RULE_FLAGS	0x16000000
 #define IMA_DIGSIG		0x01000000
 #define IMA_DIGSIG_REQUIRED	0x02000000
 #define IMA_PERMIT_DIRECTIO	0x04000000
 #define IMA_NEW_FILE		0x08000000
+#define IMA_MODSIG_ALLOWED	0x10000000
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_APPRAISE_SUBMASK)
@@ -58,6 +59,7 @@ enum evm_ima_xattr_type {
 	EVM_XATTR_HMAC,
 	EVM_IMA_XATTR_DIGSIG,
 	IMA_XATTR_DIGEST_NG,
+	IMA_MODSIG,
 	IMA_XATTR_LAST
 };
 
-- 
2.13.0

[Linux-ima-devel] [PATCH v4 6/7] ima: Store measurement after appraisal

[Thiago J. B. <bau...@li...>]

When module-style signatures appended at the end of files are supported for
IMA appraisal, the code will fallback to the xattr signature if the
appended one fails to verify.

The problem is that we don't know whether we need to fallback to the xattr
signature until the appraise step, and by then the measure step was already
completed and would need to be done again in case the template includes the
signature.

To avoid this problem, do the appraisal first so that the correct signature
is stored by the template in the measure step.

Signed-off-by: Thiago Jung Bauermann <bau...@li...>
---
 security/integrity/ima/ima_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2aebb7984437..0b4845e7248d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -244,12 +244,12 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
 		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
 
-	if (action & IMA_MEASURE)
-		ima_store_measurement(iint, file, pathname,
-				      xattr_value, xattr_len, pcr);
 	if (action & IMA_APPRAISE_SUBMASK)
 		rc = ima_appraise_measurement(func, iint, file, pathname,
 					      xattr_value, xattr_len, opened);
+	if (action & IMA_MEASURE)
+		ima_store_measurement(iint, file, pathname,
+				      xattr_value, xattr_len, pcr);
 	if (action & IMA_AUDIT)
 		ima_audit_measurement(iint, pathname);
 
-- 
2.13.0

[Linux-ima-devel] [PATCH v4 5/7] integrity: Select CONFIG_KEYS instead of depending on it

[Thiago J. B. <bau...@li...>]

This avoids a dependency cycle in CONFIG_IMA_APPRAISE_MODSIG (introduced by
a later patch in this series): it will select CONFIG_MODULE_SIG_FORMAT
which in turn selects CONFIG_KEYS. Kconfig then complains that
CONFIG_INTEGRITY_SIGNATURE depends on CONFIG_KEYS.

Signed-off-by: Thiago Jung Bauermann <bau...@li...>
---
 security/integrity/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index da9565891738..0d642e0317c7 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -17,8 +17,8 @@ if INTEGRITY
 
 config INTEGRITY_SIGNATURE
 	bool "Digital signature verification using multiple keyrings"
-	depends on KEYS
 	default n
+	select KEYS
 	select SIGNATURE
 	help
 	  This option enables digital signature verification support
-- 
2.13.0

[Linux-ima-devel] [PATCH v4 4/7] integrity: Introduce integrity_keyring_from_id

[Thiago J. B. <bau...@li...>]

IMA will need to obtain the keyring used to verify file signatures so that
it can verify the module-style signature appended to files.

Signed-off-by: Thiago Jung Bauermann <bau...@li...>
---
 security/integrity/digsig.c    | 28 +++++++++++++++++++---------
 security/integrity/integrity.h |  1 +
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 06554c448dce..bb5328ba2848 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -48,11 +48,10 @@ static bool init_keyring __initdata;
 #define restrict_link_to_ima restrict_link_by_builtin_trusted
 #endif
 
-int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
-			    const char *digest, int digestlen)
+struct key *integrity_keyring_from_id(const unsigned int id)
 {
-	if (id >= INTEGRITY_KEYRING_MAX || siglen < 2)
-		return -EINVAL;
+	if (id >= INTEGRITY_KEYRING_MAX)
+		return ERR_PTR(-EINVAL);
 
 	if (!keyring[id]) {
 		keyring[id] =
@@ -61,18 +60,29 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 			int err = PTR_ERR(keyring[id]);
 			pr_err("no %s keyring: %d\n", keyring_name[id], err);
 			keyring[id] = NULL;
-			return err;
+			return ERR_PTR(err);
 		}
 	}
 
+	return keyring[id];
+}
+
+int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
+			    const char *digest, int digestlen)
+{
+	struct key *keyring = integrity_keyring_from_id(id);
+
+	if (IS_ERR(keyring) || siglen < 2)
+		return PTR_ERR(keyring);
+
 	switch (sig[1]) {
 	case 1:
 		/* v1 API expect signature without xattr type */
-		return digsig_verify(keyring[id], sig + 1, siglen - 1,
-				     digest, digestlen);
+		return digsig_verify(keyring, sig + 1, siglen - 1, digest,
+				     digestlen);
 	case 2:
-		return asymmetric_verify(keyring[id], sig, siglen,
-					 digest, digestlen);
+		return asymmetric_verify(keyring, sig, siglen, digest,
+					 digestlen);
 	}
 
 	return -EOPNOTSUPP;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 9b1762076f38..1f8f1a31d487 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -134,6 +134,7 @@ int __init integrity_read_file(const char *path, char **data);
 
 #ifdef CONFIG_INTEGRITY_SIGNATURE
 
+struct key *integrity_keyring_from_id(const unsigned int id);
 int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 			    const char *digest, int digestlen);
 
-- 
2.13.0

[Linux-ima-devel] [PATCH v4 3/7] PKCS#7: Introduce pkcs7_get_message_sig and verify_pkcs7_message_sig

[Thiago J. B. <bau...@li...>]

IMA will need to access the digest used in the signature so that it can
verify files containing module-style appended signatures. For this purpose,
add function pkcs7_get_message_sig.

It will also need to verify an already parsed PKCS#7 message. For this
purpose, add function verify_pkcs7_message_signature which takes a struct
pkcs7_message for verification instead of the raw bytes that
verify_pkcs7_signature takes.

Signed-off-by: Thiago Jung Bauermann <bau...@li...>
---
 certs/system_keyring.c                | 60 +++++++++++++++++++++++++----------
 crypto/asymmetric_keys/pkcs7_parser.c | 12 +++++++
 include/crypto/pkcs7.h                |  2 ++
 include/linux/verification.h          | 10 ++++++
 4 files changed, 68 insertions(+), 16 deletions(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 6251d1b27f0c..6a8684959780 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -190,33 +190,26 @@ late_initcall(load_system_certificate_list);
 #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
 
 /**
- * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * verify_pkcs7_message_sig - Verify a PKCS#7-based signature on system data.
  * @data: The data to be verified (NULL if expecting internal data).
  * @len: Size of @data.
- * @raw_pkcs7: The PKCS#7 message that is the signature.
- * @pkcs7_len: The size of @raw_pkcs7.
+ * @pkcs7: The PKCS#7 message that is the signature.
  * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
  *					(void *)1UL for all trusted keys).
  * @usage: The use to which the key is being put.
  * @view_content: Callback to gain access to content.
  * @ctx: Context for callback.
  */
-int verify_pkcs7_signature(const void *data, size_t len,
-			   const void *raw_pkcs7, size_t pkcs7_len,
-			   struct key *trusted_keys,
-			   enum key_being_used_for usage,
-			   int (*view_content)(void *ctx,
-					       const void *data, size_t len,
-					       size_t asn1hdrlen),
-			   void *ctx)
+int verify_pkcs7_message_sig(const void *data, size_t len,
+			     struct pkcs7_message *pkcs7,
+			     struct key *trusted_keys,
+			     enum key_being_used_for usage,
+			     int (*view_content)(void *ctx, const void *data,
+						 size_t len, size_t asn1hdrlen),
+			     void *ctx)
 {
-	struct pkcs7_message *pkcs7;
 	int ret;
 
-	pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
-	if (IS_ERR(pkcs7))
-		return PTR_ERR(pkcs7);
-
 	/* The data should be detached - so we need to supply it. */
 	if (data && pkcs7_supply_detached_data(pkcs7, data, len) < 0) {
 		pr_err("PKCS#7 signature with non-detached data\n");
@@ -258,6 +251,41 @@ int verify_pkcs7_signature(const void *data, size_t len,
 	}
 
 error:
+	pr_devel("<==%s() = %d\n", __func__, ret);
+	return ret;
+}
+
+/**
+ * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * @data: The data to be verified (NULL if expecting internal data).
+ * @len: Size of @data.
+ * @raw_pkcs7: The PKCS#7 message that is the signature.
+ * @pkcs7_len: The size of @raw_pkcs7.
+ * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
+ *					(void *)1UL for all trusted keys).
+ * @usage: The use to which the key is being put.
+ * @view_content: Callback to gain access to content.
+ * @ctx: Context for callback.
+ */
+int verify_pkcs7_signature(const void *data, size_t len,
+			   const void *raw_pkcs7, size_t pkcs7_len,
+			   struct key *trusted_keys,
+			   enum key_being_used_for usage,
+			   int (*view_content)(void *ctx,
+					       const void *data, size_t len,
+					       size_t asn1hdrlen),
+			   void *ctx)
+{
+	struct pkcs7_message *pkcs7;
+	int ret;
+
+	pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
+	if (IS_ERR(pkcs7))
+		return PTR_ERR(pkcs7);
+
+	ret = verify_pkcs7_message_sig(data, len, pkcs7, trusted_keys, usage,
+				       view_content, ctx);
+
 	pkcs7_free_message(pkcs7);
 	pr_devel("<==%s() = %d\n", __func__, ret);
 	return ret;
diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c
index af4cd8649117..e41beda297a8 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.c
+++ b/crypto/asymmetric_keys/pkcs7_parser.c
@@ -673,3 +673,15 @@ int pkcs7_note_signed_info(void *context, size_t hdrlen,
 		return -ENOMEM;
 	return 0;
 }
+
+/**
+ * pkcs7_get_message_sig - get signature in @pkcs7
+ *
+ * This function doesn't meaningfully support messages with more than one
+ * signature. It will always return the first signature.
+ */
+const struct public_key_signature *pkcs7_get_message_sig(
+					const struct pkcs7_message *pkcs7)
+{
+	return pkcs7->signed_infos ? pkcs7->signed_infos->sig : NULL;
+}
diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
index 583f199400a3..6f51d0cb6d12 100644
--- a/include/crypto/pkcs7.h
+++ b/include/crypto/pkcs7.h
@@ -28,6 +28,8 @@ extern void pkcs7_free_message(struct pkcs7_message *pkcs7);
 extern int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,
 				  const void **_data, size_t *_datalen,
 				  size_t *_headerlen);
+extern const struct public_key_signature *pkcs7_get_message_sig(
+					const struct pkcs7_message *pkcs7);
 
 /*
  * pkcs7_trust.c
diff --git a/include/linux/verification.h b/include/linux/verification.h
index a10549a6c7cd..f04dac2728ec 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -29,6 +29,7 @@ extern const char *const key_being_used_for[NR__KEY_BEING_USED_FOR];
 #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
 
 struct key;
+struct pkcs7_message;
 
 extern int verify_pkcs7_signature(const void *data, size_t len,
 				  const void *raw_pkcs7, size_t pkcs7_len,
@@ -38,6 +39,15 @@ extern int verify_pkcs7_signature(const void *data, size_t len,
 						      const void *data, size_t len,
 						      size_t asn1hdrlen),
 				  void *ctx);
+extern int verify_pkcs7_message_sig(const void *data, size_t len,
+				    struct pkcs7_message *pkcs7,
+				    struct key *trusted_keys,
+				    enum key_being_used_for usage,
+				    int (*view_content)(void *ctx,
+							const void *data,
+							size_t len,
+							size_t asn1hdrlen),
+				    void *ctx);
 
 #ifdef CONFIG_SIGNED_PE_FILE_VERIFICATION
 extern int verify_pefile_signature(const void *pebuf, unsigned pelen,
-- 
2.13.0

[Linux-ima-devel] [PATCH v4 2/7] MODSIGN: Export module signature definitions

[Thiago J. B. <bau...@li...>]

IMA will use the module_signature format for append signatures, so export
the relevant definitions and factor out the code which verifies that the
appended signature trailer is valid.

Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
and be able to use validate_module_signature without having to depend on
CONFIG_MODULE_SIG.

Signed-off-by: Thiago Jung Bauermann <bau...@li...>
---
 include/linux/module.h           |  3 --
 include/linux/module_signature.h | 47 +++++++++++++++++++++++++
 init/Kconfig                     |  6 +++-
 kernel/Makefile                  |  2 +-
 kernel/module.c                  |  1 +
 kernel/module_signing.c          | 74 +++++++++++++++++-----------------------
 6 files changed, 85 insertions(+), 48 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index e7bdd549e527..672ad2016262 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -23,9 +23,6 @@
 #include <linux/percpu.h>
 #include <asm/module.h>
 
-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
-#define MODULE_SIG_STRING "~Module signature appended~\n"
-
 /* Not Yet Implemented */
 #define MODULE_SUPPORTED_DEVICE(name)
 
diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
new file mode 100644
index 000000000000..e80728e5b86c
--- /dev/null
+++ b/include/linux/module_signature.h
@@ -0,0 +1,47 @@
+/* Module signature handling.
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dho...@re...)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#ifndef _LINUX_MODULE_SIGNATURE_H
+#define _LINUX_MODULE_SIGNATURE_H
+
+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
+enum pkey_id_type {
+	PKEY_ID_PGP,		/* OpenPGP generated key ID */
+	PKEY_ID_X509,		/* X.509 arbitrary subjectKeyIdentifier */
+	PKEY_ID_PKCS7,		/* Signature in PKCS#7 message */
+};
+
+/*
+ * Module signature information block.
+ *
+ * The constituents of the signature section are, in order:
+ *
+ *	- Signer's name
+ *	- Key identifier
+ *	- Signature data
+ *	- Information block
+ */
+struct module_signature {
+	u8	algo;		/* Public-key crypto algorithm [0] */
+	u8	hash;		/* Digest algorithm [0] */
+	u8	id_type;	/* Key identifier type [PKEY_ID_PKCS7] */
+	u8	signer_len;	/* Length of signer's name [0] */
+	u8	key_id_len;	/* Length of key identifier [0] */
+	u8	__pad[3];
+	__be32	sig_len;	/* Length of signature data */
+};
+
+int validate_module_sig(const struct module_signature *ms, size_t file_len);
+int mod_verify_sig(const void *mod, unsigned long *_modlen);
+
+#endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/init/Kconfig b/init/Kconfig
index 8514b25db21c..c3ac1170b93a 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1734,7 +1734,7 @@ config MODULE_SRCVERSION_ALL
 config MODULE_SIG
 	bool "Module signature verification"
 	depends on MODULES
-	select SYSTEM_DATA_VERIFICATION
+	select MODULE_SIG_FORMAT
 	help
 	  Check modules for valid signatures upon load: the signature
 	  is simply appended to the module. For more information see
@@ -1749,6 +1749,10 @@ config MODULE_SIG
 	  debuginfo strip done by some packagers (such as rpmbuild) and
 	  inclusion into an initramfs that wants the module size reduced.
 
+config MODULE_SIG_FORMAT
+	def_bool n
+	select SYSTEM_DATA_VERIFICATION
+
 config MODULE_SIG_FORCE
 	bool "Require modules to be validly signed"
 	depends on MODULE_SIG
diff --git a/kernel/Makefile b/kernel/Makefile
index 4cb8e8b23c6e..d5f9748ab19f 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -56,7 +56,7 @@ obj-y += up.o
 endif
 obj-$(CONFIG_UID16) += uid16.o
 obj-$(CONFIG_MODULES) += module.o
-obj-$(CONFIG_MODULE_SIG) += module_signing.o
+obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signing.o
 obj-$(CONFIG_KALLSYMS) += kallsyms.o
 obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
 obj-$(CONFIG_CRASH_CORE) += crash_core.o
diff --git a/kernel/module.c b/kernel/module.c
index 40f983cbea81..52921fccb51a 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -19,6 +19,7 @@
 #include <linux/export.h>
 #include <linux/extable.h>
 #include <linux/moduleloader.h>
+#include <linux/module_signature.h>
 #include <linux/trace_events.h>
 #include <linux/init.h>
 #include <linux/kallsyms.h>
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 937c844bee4a..204c60d4cc9f 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -11,36 +11,38 @@
 
 #include <linux/kernel.h>
 #include <linux/errno.h>
+#include <linux/module_signature.h>
 #include <linux/string.h>
 #include <linux/verification.h>
 #include <crypto/public_key.h>
 #include "module-internal.h"
 
-enum pkey_id_type {
-	PKEY_ID_PGP,		/* OpenPGP generated key ID */
-	PKEY_ID_X509,		/* X.509 arbitrary subjectKeyIdentifier */
-	PKEY_ID_PKCS7,		/* Signature in PKCS#7 message */
-};
-
-/*
- * Module signature information block.
- *
- * The constituents of the signature section are, in order:
+/**
+ * validate_module_sig - validate that the given signature is sane
  *
- *	- Signer's name
- *	- Key identifier
- *	- Signature data
- *	- Information block
+ * @ms:		Signature to validate.
+ * @file_len:	Size of the file to which @ms is appended.
  */
-struct module_signature {
-	u8	algo;		/* Public-key crypto algorithm [0] */
-	u8	hash;		/* Digest algorithm [0] */
-	u8	id_type;	/* Key identifier type [PKEY_ID_PKCS7] */
-	u8	signer_len;	/* Length of signer's name [0] */
-	u8	key_id_len;	/* Length of key identifier [0] */
-	u8	__pad[3];
-	__be32	sig_len;	/* Length of signature data */
-};
+int validate_module_sig(const struct module_signature *ms, size_t file_len)
+{
+	if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
+		return -EBADMSG;
+	else if (ms->id_type != PKEY_ID_PKCS7) {
+		pr_err("Module is not signed with expected PKCS#7 message\n");
+		return -ENOPKG;
+	} else if (ms->algo != 0 ||
+		   ms->hash != 0 ||
+		   ms->signer_len != 0 ||
+		   ms->key_id_len != 0 ||
+		   ms->__pad[0] != 0 ||
+		   ms->__pad[1] != 0 ||
+		   ms->__pad[2] != 0) {
+		pr_err("PKCS#7 signature info has unexpected non-zero params\n");
+		return -EBADMSG;
+	}
+
+	return 0;
+}
 
 /*
  * Verify the signature on a module.
@@ -49,6 +51,7 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
 {
 	struct module_signature ms;
 	size_t modlen = *_modlen, sig_len;
+	int ret;
 
 	pr_devel("==>%s(,%zu)\n", __func__, modlen);
 
@@ -56,30 +59,15 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
 		return -EBADMSG;
 
 	memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
-	modlen -= sizeof(ms);
+
+	ret = validate_module_sig(&ms, modlen);
+	if (ret)
+		return ret;
 
 	sig_len = be32_to_cpu(ms.sig_len);
-	if (sig_len >= modlen)
-		return -EBADMSG;
-	modlen -= sig_len;
+	modlen -= sig_len + sizeof(ms);
 	*_modlen = modlen;
 
-	if (ms.id_type != PKEY_ID_PKCS7) {
-		pr_err("Module is not signed with expected PKCS#7 message\n");
-		return -ENOPKG;
-	}
-
-	if (ms.algo != 0 ||
-	    ms.hash != 0 ||
-	    ms.signer_len != 0 ||
-	    ms.key_id_len != 0 ||
-	    ms.__pad[0] != 0 ||
-	    ms.__pad[1] != 0 ||
-	    ms.__pad[2] != 0) {
-		pr_err("PKCS#7 signature info has unexpected non-zero params\n");
-		return -EBADMSG;
-	}
-
 	return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
 				      NULL, VERIFYING_MODULE_SIGNATURE,
 				      NULL, NULL);
-- 
2.13.0

[Linux-ima-devel] [PATCH v4 1/7] integrity: Introduce struct evm_xattr

[Thiago J. B. <bau...@li...>]

Even though struct evm_ima_xattr_data includes a fixed-size array to hold a
SHA1 digest, most of the code ignores the array and uses the struct to mean
"type indicator followed by data of unspecified size" and tracks the real
size of what the struct represents in a separate length variable.

The only exception to that is the EVM code, which correctly uses the
definition of struct evm_ima_xattr_data.

This patch makes this explicit in the code by removing the length
specification from the array in struct evm_ima_xattr_data. It also changes
the name of the element from digest to data, since in most places the array
doesn't hold a digest.

A separate struct evm_xattr is introduced, with the original definition of
evm_ima_xattr_data to be used in the places that actually expect that
definition.

Signed-off-by: Thiago Jung Bauermann <bau...@li...>
---
 security/integrity/evm/evm_crypto.c   |  4 ++--
 security/integrity/evm/evm_main.c     | 10 +++++-----
 security/integrity/ima/ima_appraise.c |  7 ++++---
 security/integrity/integrity.h        |  5 +++++
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 1d32cd20009a..6ee25d7e5141 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -252,13 +252,13 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 			const char *xattr_value, size_t xattr_value_len)
 {
 	struct inode *inode = d_backing_inode(dentry);
-	struct evm_ima_xattr_data xattr_data;
+	struct evm_xattr xattr_data;
 	int rc = 0;
 
 	rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
 			   xattr_value_len, xattr_data.digest);
 	if (rc == 0) {
-		xattr_data.type = EVM_XATTR_HMAC;
+		xattr_data.data.type = EVM_XATTR_HMAC;
 		rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
 					   &xattr_data,
 					   sizeof(xattr_data), 0);
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 063d38aef64e..536694499515 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -116,7 +116,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 					     struct integrity_iint_cache *iint)
 {
 	struct evm_ima_xattr_data *xattr_data = NULL;
-	struct evm_ima_xattr_data calc;
+	struct evm_xattr calc;
 	enum integrity_status evm_status = INTEGRITY_PASS;
 	int rc, xattr_len;
 
@@ -147,7 +147,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 	/* check value type */
 	switch (xattr_data->type) {
 	case EVM_XATTR_HMAC:
-		if (xattr_len != sizeof(struct evm_ima_xattr_data)) {
+		if (xattr_len != sizeof(struct evm_xattr)) {
 			evm_status = INTEGRITY_FAIL;
 			goto out;
 		}
@@ -155,7 +155,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 				   xattr_value_len, calc.digest);
 		if (rc)
 			break;
-		rc = crypto_memneq(xattr_data->digest, calc.digest,
+		rc = crypto_memneq(xattr_data->data, calc.digest,
 			    sizeof(calc.digest));
 		if (rc)
 			rc = -EINVAL;
@@ -467,7 +467,7 @@ int evm_inode_init_security(struct inode *inode,
 				 const struct xattr *lsm_xattr,
 				 struct xattr *evm_xattr)
 {
-	struct evm_ima_xattr_data *xattr_data;
+	struct evm_xattr *xattr_data;
 	int rc;
 
 	if (!evm_initialized || !evm_protected_xattr(lsm_xattr->name))
@@ -477,7 +477,7 @@ int evm_inode_init_security(struct inode *inode,
 	if (!xattr_data)
 		return -ENOMEM;
 
-	xattr_data->type = EVM_XATTR_HMAC;
+	xattr_data->data.type = EVM_XATTR_HMAC;
 	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
 	if (rc < 0)
 		goto out;
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 809ba70fbbbf..87d2b601cf8e 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -156,7 +156,8 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 		return sig->hash_algo;
 		break;
 	case IMA_XATTR_DIGEST_NG:
-		ret = xattr_value->digest[0];
+		/* first byte contains algorithm id */
+		ret = xattr_value->data[0];
 		if (ret < HASH_ALGO__LAST)
 			return ret;
 		break;
@@ -164,7 +165,7 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 		/* this is for backward compatibility */
 		if (xattr_len == 21) {
 			unsigned int zero = 0;
-			if (!memcmp(&xattr_value->digest[16], &zero, 4))
+			if (!memcmp(&xattr_value->data[16], &zero, 4))
 				return HASH_ALGO_MD5;
 			else
 				return HASH_ALGO_SHA1;
@@ -253,7 +254,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 			/* xattr length may be longer. md5 hash in previous
 			   version occupied 20 bytes in xattr, instead of 16
 			 */
-			rc = memcmp(&xattr_value->digest[hash_start],
+			rc = memcmp(&xattr_value->data[hash_start],
 				    iint->ima_hash->digest,
 				    iint->ima_hash->length);
 		else
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index a53e7e4ab06c..9b1762076f38 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -63,6 +63,11 @@ enum evm_ima_xattr_type {
 
 struct evm_ima_xattr_data {
 	u8 type;
+	u8 data[];
+} __packed;
+
+struct evm_xattr {
+	struct evm_ima_xattr_data data;
 	u8 digest[SHA1_DIGEST_SIZE];
 } __packed;
 
-- 
2.13.0

[Linux-ima-devel] [PATCH v4 0/7] Appended signatures support for IMA appraisal

[Thiago J. B. <bau...@li...>]

Hello,

This version implements an approach suggested by Mimi Zohar, which is that
if the modsig is valid, ima_appraise_measurement will now copy the hash
calculated by the verification process into the iint cache. This ensures
that the hash will appear in the measurement list and used to extend the
TPM PCR.

Original cover letter:

On the OpenPOWER platform, secure boot and trusted boot are being
implemented using IMA for taking measurements and verifying signatures.
Since the kernel image on Power servers is an ELF binary, kernels are
signed using the scripts/sign-file tool and thus use the same signature
format as signed kernel modules.

This patch series adds support in IMA for verifying those signatures.
It adds flexibility to OpenPOWER secure boot, because it allows it to boot
kernels with the signature appended to them as well as kernels where the
signature is stored in the IMA extended attribute.

Since modsig is only supported on some specific hooks which don't get
called often (cf. ima_hook_supports_modsig), it's possible to always check
for the presence of an appended modsig before looking for the xattr sig. In
that case, the policy doesn't need to be changed to support the modsig
keyword. Is that preferable than requiring the policy to explicitly allow a
modsig like this code does?

I tested these patches with EVM and I believe they don't break it and
things work as expected, but I'm not really familiar with EVM and its use
cases so this should be taken with a grain of salt.

I also verified that the code correctly recalculates the file hash if the
modsig verification fails and the file also has an xattr signature which
uses a different hash algorithm.

These patches apply on top of today's linux-security/next.

Changes since v3:
- Patch "integrity: Introduce struct evm_hmac_xattr"
  - Renamed new struct to evm_xattr.
  - Define struct evm_xattr using struct evm_ima_xattr_data, and moved it
    from evm.h to integrity.h (suggested by Mimi Zohar).
- Patch "PKCS#7: Introduce verify_pkcs7_message_sig"
  - Also introduce pkcs7_get_message_sig.
- Patch "ima: Support appended signatures for appraisal"
  - Moved check for buffer presence and size from ima_appraise_measurement
    to ima_read_modsig (suggested by Mimi Zohar).
  - Factored out handling of ima_read_xattr return value into
    process_xattr_error in ima_appraise_measurement so that it can be used
    if the modsig verification fails.
  - Pass NULL xattr_value to evm_verifyxattr even in the case of xattr
    signature in ima_appraise_measurement (suggested by Mimi Zohar).
  - Use switch statement provided by Mimi Zohar to check result of
    evm_verifyxattr.
  - If the modsig verification succeeds, copy the hash calculated during
    the verification to the iint cache (suggested by Mimi Zohar).
  - Substitute recursion in ima_appraise_measurement by a goto statement
    back to the main switch statement (suggested by Mimi Zohar).

Changes since v2:
- Patch "MODSIGN: Export module signature definitions."
  - Put change introducing function verify_pkcs7_message_signature into
    its own patch (suggested by Mimi Zohar).
  - Shortened validate_module_signature to validate_module_sig.

- Patch "PKCS#7: Introduce verify_pkcs7_message_sig"
  - New patch in this series.
  - Shortened verify_pkcs7_message_signature to verify_pkcs7_message_sig.

- Patch "integrity: Introduce integrity_keyring_from_id"
  - New patch in this series.

- Patch "integrity: Select CONFIG_KEYS instead of depending on it"
  - New patch in this series.

- Patch "ima: Store measurement after appraisal"
  - New patch in this series.
  - Instead of creating function measure_and_appraise, simply call
    ima_appraise_measurement before ima_store_measurement in
    process_measurement (suggested by Mimi Zohar).

- Patch "ima: Support appended signatures for appraisal"
  - Put change introducing function integrity_keyring_from_id into
    its own patch (suggested by Mimi Zohar).
  - Put change to select CONFIG_KEYS in its own patch.
  - Put change in the order of measure and appraise steps into
    its own patch (suggested by Mimi Zohar).
  - Add buf and size arguments to ima_appraise_measurement. Also,
    pass xattr_value and xattr_len by reference so that the function can
    change them to point to the modsig.
  - Don't pass buf_len by reference in ima_read_modsig. It doesn't need
    to be changed anymore now that the hash calculated by the collect step
    covers the whole file instead of skipping the modsig at the end.
  - Don't add pkcs7_get_message_sig. It's not necessary anymore. Ditto for
    ima_get_modsig_hash_algo.
  - Don't change ima_collect_measurement anymore to recalculate the file
    hash if the algorithm is different, since now it doesn't have anything
    to do with the hash used by the modsig.
  - Don't change ima_get_hash_alog anymore to obtain the hash algo used by
    the modsig, since it isn't used in the collect step.
  - Change ima_appraise_measurement to check whether there is a modsig
    before verifying the xattr if the policy rule allows a modsig.
  - Use separate if clause to check result of evm_verifyxattr when
    appraising modsig (suggested by Mimi Zohar).
  - Use bool variable in ima_inode_setxattr to make code clearer (suggested
    by Mimi Zohar).
  - Don't define pr_fmt in ima_main.c.
  - Renamed struct signature_modsig_hdr to modsig_hdr.

Changes since v1:
- Patch "integrity: Small code improvements"
  - Add missing #endif comment in ima.h.

- Patch "ima: Tidy up constant strings"
  - Squashed into previous patch.

- Patch "ima: Simplify policy_func_show."
  - Generate ima_hooks enum and func_tokens array from a single macro.
    (suggested by Mimi)
  - Further simplify policy_func_show by not using the printf format string
    from the policy_tokens table.

- Patch "integrity: Introduce struct evm_hmac_xattr"
  - New patch.

- Patch "MODSIGN: Export module signature definitions."
  - Add function verify_pkcs7_message_signature which takes a
    struct pkcs7_message.
  - Move MODULE_SIG_STRING definition from <linux/module.h> to
    <linux/module_signature.h>.

- Patch "ima: Support appended signatures for appraisal"
  - Changed name from appended_sig to modsig. (suggested by Mehmet Kayaalp)
  - Don't add key_being_used_for value VERIFYING_KEXEC_CMS_SIGNATURE. Use
    existing VERIFYING_MODULE_SIGNATURE. (suggested by Mimi)
  - Moved modsig code to its own file. (suggested by Mimi)
  - Added new xattr "subtype" IMA_MODSIG. (suggested by Mimi)
  - Check whether a hook supports modsig when the policy is being parsed.
    (suggested by Mimi)
  - If the modsig verification fails, look for an xattr signature.
    (suggested by Mimi)
  - Add integrity_keyring_from_id function.
  - Put modsig to measurement list if the template requires the signature
    contents. (suggested by Mimi).

Thiago Jung Bauermann (7):
  integrity: Introduce struct evm_xattr
  MODSIGN: Export module signature definitions
  PKCS#7: Introduce pkcs7_get_message_sig and verify_pkcs7_message_sig
  integrity: Introduce integrity_keyring_from_id
  integrity: Select CONFIG_KEYS instead of depending on it
  ima: Store measurement after appraisal
  ima: Support module-style appended signatures for appraisal

 certs/system_keyring.c                    |  60 +++++++---
 crypto/asymmetric_keys/pkcs7_parser.c     |  12 ++
 include/crypto/pkcs7.h                    |   2 +
 include/linux/module.h                    |   3 -
 include/linux/module_signature.h          |  47 ++++++++
 include/linux/verification.h              |  10 ++
 init/Kconfig                              |   6 +-
 kernel/Makefile                           |   2 +-
 kernel/module.c                           |   1 +
 kernel/module_signing.c                   |  74 +++++-------
 security/integrity/Kconfig                |   2 +-
 security/integrity/digsig.c               |  28 +++--
 security/integrity/evm/evm_crypto.c       |   4 +-
 security/integrity/evm/evm_main.c         |  10 +-
 security/integrity/ima/Kconfig            |  13 +++
 security/integrity/ima/Makefile           |   1 +
 security/integrity/ima/ima.h              |  70 ++++++++++-
 security/integrity/ima/ima_appraise.c     | 185 +++++++++++++++++++++++++-----
 security/integrity/ima/ima_main.c         |   9 +-
 security/integrity/ima/ima_modsig.c       | 178 ++++++++++++++++++++++++++++
 security/integrity/ima/ima_policy.c       |  26 +++--
 security/integrity/ima/ima_template_lib.c |  14 ++-
 security/integrity/integrity.h            |  10 +-
 23 files changed, 634 insertions(+), 133 deletions(-)
 create mode 100644 include/linux/module_signature.h
 create mode 100644 security/integrity/ima/ima_modsig.c

-- 
2.13.0

Re: [Linux-ima-devel] [PATCH v4 2/5] ima: use fs method to read integrity data [updated]

[Mimi Z. <zo...@li...>]

On Thu, 2017-08-03 at 12:56 +0200, Jan Kara wrote:
> On Wed 02-08-17 13:11:52, Mimi Zohar wrote:
> > On Wed, 2017-08-02 at 10:01 +0200, Jan Kara wrote:
> > > On Tue 01-08-17 16:24:30, Mimi Zohar wrote:
> > > > From: Christoph Hellwig <hc...@ls...>
> > > > 
> > > > Add a new ->integrity_read file operation to read data for integrity
> > > > hash collection.  This is defined to be equivalent to ->read_iter,
> > > > except that it will be called with the i_rwsem held exclusively.
> > > > 
> > > > Signed-off-by: Christoph Hellwig <hc...@ls...>
> > > > Cc: Matthew Garrett <mat...@ne...>
> > > > Cc: Jan Kara <ja...@su...>
> > > > Cc: "Theodore Ts'o" <ty...@mi...>
> > > > Cc: Andreas Dilger <adi...@di...>
> > > > Cc: Jaegeuk Kim <ja...@ke...>
> > > > Cc: Chao Yu <yu...@hu...>
> > > > Cc: Steven Whitehouse <swh...@re...>
> > > > Cc: Bob Peterson <rpe...@re...>
> > > > Cc: David Woodhouse <dw...@in...>
> > > > Cc: Dave Kleikamp <sh...@ke...>
> > > > Cc: Ryusuke Konishi <kon...@la...>
> > > > Cc: Mark Fasheh <mf...@ve...>
> > > > Cc: Joel Becker <jl...@ev...>
> > > > Cc: Richard Weinberger <ri...@no...>
> > > > Cc: "Darrick J. Wong" <dar...@or...>
> > > > Cc: Hugh Dickins <hu...@go...>
> > > > Cc: Chris Mason <cl...@fb...>
> > > > Signed-off-by: Mimi Zohar <zo...@li...>
> > > 
> > > ...
> > > 
> > > > +static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb,
> > > > +					     struct iov_iter *to)
> > > > +{
> > > > +	struct inode *inode = file_inode(iocb->ki_filp);
> > > > +	int o_direct = iocb->ki_flags & IOCB_DIRECT;
> > > > +
> > > > +	lockdep_assert_held(&inode->i_rwsem);
> > > > +	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
> > > > +		return -EIO;
> > > > +
> > > > +	if (!iov_iter_count(to))
> > > > +		return 0; /* skip atime */
> > > > +
> > > > +#ifdef CONFIG_FS_DAX
> > > > +	if (IS_DAX(inode))
> > > > +		return dax_iomap_rw(iocb, to, &ext4_iomap_ops);
> > > > +#endif
> > > > +	if (o_direct)
> > > > +		return -EINVAL;
> > > > +	return generic_file_read_iter(iocb, to);
> > > > +}
> > > 
> > > I have noticed this o_direct check - why is it only in ext4 and shouldn't
> > > rather higher layers make sure IOCB_DIRECT iocbs cannot reach
> > > .integrity_read() methods?
> >  
> > This failure happens when opening a file with O_DIRECT on a block
> > device that does not support dax (eg. loop). xfs makes it to here too,
> > but the call to generic_file_read_iter() fails properly with -EINVAL. 
> > (Only tested on those filesystems included that support dax (eg. ext2,
> > ext4, and xfs).)
> 
> Well, yes, ext2 and ext4 will silently fall back to buffered read (as do
> pretty much all filesystems except for XFS). However I fail to see why IMA
> should care (which is probably due to my lack of knowledge about IMA).
> Is O_DIRECT somehow excepted from IMA? But then why it is not for DAX?

Thank you for the explanation!  (I was confused about the relationship
between O_DIRECT and DAX.)  You're correct.  IMA does not support
O_DIRECT in the buffered case for two reasons, locking and updating
the file hash, which are described in commit f9b2a735bddd "ima: audit
log files opened with O_DIRECT flag".  After reverting this commit,
the O_DIRECT check is needed before calling generic_file_read_iter().
 Most likely the same would need to be done for other filesystems that
support O_DIRECT.  Probably a generic_integrity_file_read_iter()
should be defined.
 
For DAX, unlike do_blockdev_direct_IO() which takes the lock, reading
the file with O_DIRECT is fine, as dax_iomap_rw() only checks that the
lock has been taken.  Assuming the file system is mounted with
i_version, the file hash is updated properly.

Mimi

Re: [Linux-ima-devel] [PATCH v3 7/7] ima: Support module-style appended signatures for appraisal

[Thiago J. B. <bau...@li...>]

Mimi Zohar <zo...@li...> writes:

> On Wed, 2017-08-02 at 18:52 -0400, Mimi Zohar wrote:
>> On Wed, 2017-08-02 at 14:42 -0300, Thiago Jung Bauermann wrote:
>> > Mimi Zohar <zo...@li...> writes:
>
>> > >> @@ -229,8 +251,24 @@ int ima_appraise_measurement(enum ima_hooks func,
>> > >>  		goto out;
>> > >>  	}
>> > >> 
>> > >> -	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
>> > >> -	if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
>> > >> +	/*
>> > >> +	 * Appended signatures aren't protected by EVM but we still call
>> > >> +	 * evm_verifyxattr to check other security xattrs, if they exist.
>> > >> +	 */
>> > >> +	if (appraising_modsig) {
>> > >> +		xattr_value_evm = NULL;
>> > >> +		xattr_len_evm = 0;
>> > >> +	} else {
>> > >> +		xattr_value_evm = xattr_value;
>> > >> +		xattr_len_evm = xattr_len;
>> > >> +	}
>> > >> +
>> > >> +	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value_evm,
>> > >> +				 xattr_len_evm, iint);
>> > >> +	if (appraising_modsig && status == INTEGRITY_FAIL) {
>> > >> +		cause = "invalid-HMAC";
>> > >> +		goto out;
>> > >
>> > > "modsig" is special, because having any security xattrs is not
>> > > required. This test doesn't prevent status from being set to
>> > > "missing-HMAC". This test is redundant with the original tests below.
>> > 
>> > Indeed, that is wrong. I'm still a bit fuzzy about how EVM works and how
>> > it interacts with IMA. The only way I can think of singling out modsig
>> > without reintroduced the complex expression you didn't like in v2 is as
>> > below. What do you think?
>> 
>> The original code, without any extra tests, should be fine.
>
> There is one major difference.
>
> EVM verifies a file's metadata has not been modified based on either
> an HMAC or signature stored as security.evm. Prior to the appended
> signatures patch set, all files in policy required a security.evm
> xattr. With IMA enabled we could guarantee that at least one security
> xattr existed. The only exception were new files, which hadn't yet
> been labeled.
>
> With appended signatures, there is now no guarantee that at least one
> security xattr exists.
>
> Perhaps the code snippet below will help clarify the meaning of the
> integrity_status results.
>
>     switch (status) {
> case INTEGRITY_PASS:
> case INTEGRITY_UNKNOWN:   
>        break;		
>     case INTEGRITY_NOXATTRS:/* no EVM protected xattrs */
> if (appraising_modsig)
> break;
> case INTEGRITY_NOLABEL:/* no security.evm xattr */
> cause = "missing-HMAC";
> fail = 1;
> break;
> case INTEGRITY_FAIL:/* invalid HMAC/signature */
> default:
> cause = "invalid-HMAC";
> fail = 1;
> break;
> }

Thanks! I'll use the switch above in the next version of the patch.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [Linux-ima-devel] [PATCH v3 7/7] ima: Support module-style appended signatures for appraisal

[Mimi Z. <zo...@li...>]

On Wed, 2017-08-02 at 18:52 -0400, Mimi Zohar wrote:
> On Wed, 2017-08-02 at 14:42 -0300, Thiago Jung Bauermann wrote:
> > Mimi Zohar <zo...@li...> writes:

> > >> @@ -229,8 +251,24 @@ int ima_appraise_measurement(enum ima_hooks func,
> > >>  		goto out;
> > >>  	}
> > >> 
> > >> -	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
> > >> -	if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
> > >> +	/*
> > >> +	 * Appended signatures aren't protected by EVM but we still call
> > >> +	 * evm_verifyxattr to check other security xattrs, if they exist.
> > >> +	 */
> > >> +	if (appraising_modsig) {
> > >> +		xattr_value_evm = NULL;
> > >> +		xattr_len_evm = 0;
> > >> +	} else {
> > >> +		xattr_value_evm = xattr_value;
> > >> +		xattr_len_evm = xattr_len;
> > >> +	}
> > >> +
> > >> +	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value_evm,
> > >> +				 xattr_len_evm, iint);
> > >> +	if (appraising_modsig && status == INTEGRITY_FAIL) {
> > >> +		cause = "invalid-HMAC";
> > >> +		goto out;
> > >
> > > "modsig" is special, because having any security xattrs is not
> > > required. This test doesn't prevent status from being set to
> > > "missing-HMAC". This test is redundant with the original tests below.
> > 
> > Indeed, that is wrong. I'm still a bit fuzzy about how EVM works and how
> > it interacts with IMA. The only way I can think of singling out modsig
> > without reintroduced the complex expression you didn't like in v2 is as
> > below. What do you think?
> 
> The original code, without any extra tests, should be fine.

There is one major difference.

EVM verifies a file's metadata has not been modified based on either
an HMAC or signature stored as security.evm.  Prior to the appended
signatures patch set, all files in policy required a security.evm
xattr. With IMA enabled we could guarantee that at least one security
xattr existed.  The only exception were new files, which hadn't yet
been labeled. 

With appended signatures, there is now no guarantee that at least one
security xattr exists.

Perhaps the code snippet below will help clarify the meaning of the
integrity_status results. 

        switch (status) {
        case INTEGRITY_PASS:
        case INTEGRITY_UNKNOWN:      
              break;		
        case INTEGRITY_NOXATTRS:        /* no EVM protected xattrs */
                if (appraising_modsig)
                        break;
        case INTEGRITY_NOLABEL:         /* no security.evm xattr */
                cause = "missing-HMAC";
                fail = 1;
                break;
        case INTEGRITY_FAIL:            /* invalid HMAC/signature */
        default:
                cause = "invalid-HMAC";
                fail = 1;
                break;
        }

Mimi

Re: [Linux-ima-devel] [PATCH v4 2/5] ima: use fs method to read integrity data [updated]

[Jan K. <ja...@su...>]

On Wed 02-08-17 13:11:52, Mimi Zohar wrote:
> On Wed, 2017-08-02 at 10:01 +0200, Jan Kara wrote:
> > On Tue 01-08-17 16:24:30, Mimi Zohar wrote:
> > > From: Christoph Hellwig <hc...@ls...>
> > > 
> > > Add a new ->integrity_read file operation to read data for integrity
> > > hash collection.  This is defined to be equivalent to ->read_iter,
> > > except that it will be called with the i_rwsem held exclusively.
> > > 
> > > Signed-off-by: Christoph Hellwig <hc...@ls...>
> > > Cc: Matthew Garrett <mat...@ne...>
> > > Cc: Jan Kara <ja...@su...>
> > > Cc: "Theodore Ts'o" <ty...@mi...>
> > > Cc: Andreas Dilger <adi...@di...>
> > > Cc: Jaegeuk Kim <ja...@ke...>
> > > Cc: Chao Yu <yu...@hu...>
> > > Cc: Steven Whitehouse <swh...@re...>
> > > Cc: Bob Peterson <rpe...@re...>
> > > Cc: David Woodhouse <dw...@in...>
> > > Cc: Dave Kleikamp <sh...@ke...>
> > > Cc: Ryusuke Konishi <kon...@la...>
> > > Cc: Mark Fasheh <mf...@ve...>
> > > Cc: Joel Becker <jl...@ev...>
> > > Cc: Richard Weinberger <ri...@no...>
> > > Cc: "Darrick J. Wong" <dar...@or...>
> > > Cc: Hugh Dickins <hu...@go...>
> > > Cc: Chris Mason <cl...@fb...>
> > > Signed-off-by: Mimi Zohar <zo...@li...>
> > 
> > ...
> > 
> > > +static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb,
> > > +					     struct iov_iter *to)
> > > +{
> > > +	struct inode *inode = file_inode(iocb->ki_filp);
> > > +	int o_direct = iocb->ki_flags & IOCB_DIRECT;
> > > +
> > > +	lockdep_assert_held(&inode->i_rwsem);
> > > +	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
> > > +		return -EIO;
> > > +
> > > +	if (!iov_iter_count(to))
> > > +		return 0; /* skip atime */
> > > +
> > > +#ifdef CONFIG_FS_DAX
> > > +	if (IS_DAX(inode))
> > > +		return dax_iomap_rw(iocb, to, &ext4_iomap_ops);
> > > +#endif
> > > +	if (o_direct)
> > > +		return -EINVAL;
> > > +	return generic_file_read_iter(iocb, to);
> > > +}
> > 
> > I have noticed this o_direct check - why is it only in ext4 and shouldn't
> > rather higher layers make sure IOCB_DIRECT iocbs cannot reach
> > .integrity_read() methods?
>  
> This failure happens when opening a file with O_DIRECT on a block
> device that does not support dax (eg. loop). xfs makes it to here too,
> but the call to generic_file_read_iter() fails properly with -EINVAL. 
> (Only tested on those filesystems included that support dax (eg. ext2,
> ext4, and xfs).)

Well, yes, ext2 and ext4 will silently fall back to buffered read (as do
pretty much all filesystems except for XFS). However I fail to see why IMA
should care (which is probably due to my lack of knowledge about IMA).
Is O_DIRECT somehow excepted from IMA? But then why it is not for DAX?

								Honza

-- 
Jan Kara <ja...@su...>
SUSE Labs, CR

Re: [Linux-ima-devel] [PATCH v3 7/7] ima: Support module-style appended signatures for appraisal

[Mimi Z. <zo...@li...>]

On Wed, 2017-08-02 at 14:42 -0300, Thiago Jung Bauermann wrote:
> Mimi Zohar <zo...@li...> writes:
> 
> > On Thu, 2017-07-06 at 19:17 -0300, Thiago Jung Bauermann wrote:
> >> --- a/security/integrity/ima/ima_appraise.c
> >> +++ b/security/integrity/ima/ima_appraise.c
> >> @@ -200,18 +200,40 @@ int ima_read_xattr(struct dentry *dentry,
> >>   */
> >>  int ima_appraise_measurement(enum ima_hooks func,
> >>  			     struct integrity_iint_cache *iint,
> >> -			     struct file *file, const unsigned char *filename,
> >> -			     struct evm_ima_xattr_data *xattr_value,
> >> -			     int xattr_len, int opened)
> >> +			     struct file *file, const void *buf, loff_t size,
> >> +			     const unsigned char *filename,
> >> +			     struct evm_ima_xattr_data **xattr_value_,
> >> +			     int *xattr_len_, int opened)
> >>  {
> >>  	static const char op[] = "appraise_data";
> >>  	char *cause = "unknown";
> >>  	struct dentry *dentry = file_dentry(file);
> >>  	struct inode *inode = d_backing_inode(dentry);
> >>  	enum integrity_status status = INTEGRITY_UNKNOWN;
> >> -	int rc = xattr_len, hash_start = 0;
> >> +	struct evm_ima_xattr_data *xattr_value = *xattr_value_;
> >> +	int xattr_len = *xattr_len_, rc = xattr_len, hash_start = 0;
> >> +	bool appraising_modsig = false;
> >> +	void *xattr_value_evm;
> >> +	size_t xattr_len_evm;
> >> +
> >> +	if (iint->flags & IMA_MODSIG_ALLOWED) {
> >> +		/*
> >> +		 * Not supposed to happen. Hooks that support modsig are
> >> +		 * whitelisted when parsing the policy using
> >> +		 * ima_hooks_supports_modsig.
> >> +		 */
> >> +		if (!buf || !size)
> >> +			WARN_ONCE(true, "%s doesn't support modsig\n",
> >> +				  func_tokens[func]);
> >
> > ima _appraise_measurement() is getting kind of long. Is there any
> > reason we can't move this comment and test to ima_read_modsig()?
> 
> I didn't do that because then I would need to pass func as an argument
> to ima_read_modsig just to print the warning above. But it does simplify
> the code so it may be worth it. I'll make that change in v4.

Makes sense.

> >> @@ -229,8 +251,24 @@ int ima_appraise_measurement(enum ima_hooks func,
> >>  		goto out;
> >>  	}
> >> 
> >> -	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
> >> -	if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
> >> +	/*
> >> +	 * Appended signatures aren't protected by EVM but we still call
> >> +	 * evm_verifyxattr to check other security xattrs, if they exist.
> >> +	 */
> >> +	if (appraising_modsig) {
> >> +		xattr_value_evm = NULL;
> >> +		xattr_len_evm = 0;
> >> +	} else {
> >> +		xattr_value_evm = xattr_value;
> >> +		xattr_len_evm = xattr_len;
> >> +	}
> >> +
> >> +	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value_evm,
> >> +				 xattr_len_evm, iint);
> >> +	if (appraising_modsig && status == INTEGRITY_FAIL) {
> >> +		cause = "invalid-HMAC";
> >> +		goto out;
> >
> > "modsig" is special, because having any security xattrs is not
> > required. This test doesn't prevent status from being set to
> > "missing-HMAC". This test is redundant with the original tests below.
> 
> Indeed, that is wrong. I'm still a bit fuzzy about how EVM works and how
> it interacts with IMA. The only way I can think of singling out modsig
> without reintroduced the complex expression you didn't like in v2 is as
> below. What do you think?

The original code, without any extra tests, should be fine.

> 
> @@ -229,8 +241,25 @@ int ima_appraise_measurement(enum ima_hooks func,
>  		goto out;
>  	}
> 
> -	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
> -	if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
> +	/*
> +	 * Appended signatures aren't protected by EVM but we still call
> +	 * evm_verifyxattr to check other security xattrs, if they exist.
> +	 */
> +	if (appraising_modsig) {
> +		xattr_value_evm = NULL;
> +		xattr_len_evm = 0;
> +	} else {
> +		xattr_value_evm = xattr_value;
> +		xattr_len_evm = xattr_len;
> +	}
> +
> +	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value_evm,
> +				 xattr_len_evm, iint);
> +	if (appraising_modsig && (status == INTEGRITY_NOLABEL
> +				  || status == INTEGRITY_NOXATTRS))
> +		/* It's ok if there's no xattr in the case of modsig. */
> +		;
> +	else if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN) {
>  		if ((status == INTEGRITY_NOLABEL)
>  		    || (status == INTEGRITY_NOXATTRS))
>  			cause = "missing-HMAC";
> 
> >> +	} else if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN) {
> >>  		if ((status == INTEGRITY_NOLABEL)
> >>  		    || (status == INTEGRITY_NOXATTRS))
> >>  			cause = "missing-HMAC";
> >> @@ -281,6 +319,43 @@ int ima_appraise_measurement(enum ima_hooks func,
> >>  			status = INTEGRITY_PASS;
> >>  		}
> >
> > Calling evm_verifyxattr() with the IMA xattr value prevents EVM from
> > having to re-read the IMA xattr, but isn't necessary.On modsig
> > signature verification failure, calling evm_verifyxattr() a second
> > time isn't necessary.
> 
> So even for the IMA xattr sig case, the evm_verifyxattr call in
> ima_appraise_measurement is an optimization and can be skipped?

Right, it is just an optimization.  The evm xattr needs to be verified
just once.

> >> +	case IMA_MODSIG:
> >> +		/*
> >> +		 * To avoid being tricked into recursion, we don't allow a
> >> +		 * modsig stored in the xattr.
> >> +		 */
> >> +		if (!appraising_modsig) {
> >> +			status = INTEGRITY_UNKNOWN;
> >> +			cause = "unknown-ima-data";
> >> +
> >> +			break;
> >> +		}
> >> +
> >> +		rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, xattr_value);
> >> +		if (!rc) {
> >> +			iint->flags |= IMA_DIGSIG;
> >> +			status = 
> >> +
> >> +			kfree(*xattr_value_);
> >> +			*xattr_value_ = xattr_value;
> >> +			*xattr_len_ = xattr_len;
> >> +
> >> +			break;
> >> +		}
> >
> > When including the appended signature in the measurement list, the
> > corresponding file hash needs to be included in the measurement list,
> > which might be different than the previously calculated file hash
> > based on the hash algorithm as defined in the IMA xattr.
> >
> > Including the file hash and signature in the measurement list allows
> I> the attestation server, with just a public key, to verify the file
> > signature against the file hash. No need for a white list.
> >
> > ima_modsig_verify() must calculate the file hash in order to verify
> > the file signature. This file hash value somehow needs to be returned
> > in order for it to be included in the measurement list.
> 
> In that case, patch 6/7 "ima: Store measurement after appraisal" isn't
> enough and we have to go back to v2's change in ima_main.c which ties
> together the collect and appraise steps in process_measurement (In that
> version I called the function measure_and_appraise but it should be
> called collect_and_appraise instead). That is because if the modsig
> verification fails, the hash needs to be recalculated for the xattr
> signature verification.

> Either that, or I add another call to ima_collect_measurement inside
> ima_appraise_measurement if the modsig verification fails. Which do you
> prefer?

The file hash (without the appended signature) is already being
calculated by verify_pkcs7_message_sig().  There's no reason to
calculate it twice.

If the appended signature verification succeeds, that means the file
hash stored in the appended signature was valid.  Somehow we need
access to sig->digest, sig->digest_size and sig->hash_algo, which was
compared against the calculated hash.  Refer to
public_key_verify_signature().

Mimi

Re: [Linux-ima-devel] [RFC PATCH 3/5] ima: mamespace audit status flags

[Tycho A. <ty...@do...>]

On Tue, Aug 01, 2017 at 01:25:31PM -0400, Mehmet Kayaalp wrote:
> >> +unsigned long iint_flags(struct integrity_iint_cache *iint,
> >> +			 struct ns_status *status)
> >> +{
> >> +	if (!status)
> >> +		return iint->flags;
> >> +
> >> +	return iint->flags & (status->flags & IMA_NS_STATUS_FLAGS);
> > 
> > Just to confirm, is there any situation where:
> > 
> >    iint->flags & IMA_NS_STATUS_FLAGS != status->flags & IMA_NS_STATUS_FLAGS
> > 
> > ? i.e. can this line just be:
> > 
> >    return status->flags & IMA_NS_STATUS_FLAGS;
> > 
> 
> As Guilherme had pointed out, the first & should be |.

Sorry, that mail got filtered somehow, thanks. Per your discussion, I
guess the most defensive way is:

iint->flags & ~IMA_NS_STATUS_FLAGS | status->flags & IMA_NS_STATUS_FLAGS

in case something comes along and sets IMA_AUDITED on the root iint,
we don't want it to propagate to this ns' status unnecessarily.

Anyway, thanks!

Tycho

Re: [Linux-ima-devel] [PATCH v3 7/7] ima: Support module-style appended signatures for appraisal

[Thiago J. B. <bau...@li...>]

Mimi Zohar <zo...@li...> writes:

> On Thu, 2017-07-06 at 19:17 -0300, Thiago Jung Bauermann wrote:
>> --- a/security/integrity/ima/ima_appraise.c
>> +++ b/security/integrity/ima/ima_appraise.c
>> @@ -200,18 +200,40 @@ int ima_read_xattr(struct dentry *dentry,
>>   */
>>  int ima_appraise_measurement(enum ima_hooks func,
>>  			     struct integrity_iint_cache *iint,
>> -			     struct file *file, const unsigned char *filename,
>> -			     struct evm_ima_xattr_data *xattr_value,
>> -			     int xattr_len, int opened)
>> +			     struct file *file, const void *buf, loff_t size,
>> +			     const unsigned char *filename,
>> +			     struct evm_ima_xattr_data **xattr_value_,
>> +			     int *xattr_len_, int opened)
>>  {
>>  	static const char op[] = "appraise_data";
>>  	char *cause = "unknown";
>>  	struct dentry *dentry = file_dentry(file);
>>  	struct inode *inode = d_backing_inode(dentry);
>>  	enum integrity_status status = INTEGRITY_UNKNOWN;
>> -	int rc = xattr_len, hash_start = 0;
>> +	struct evm_ima_xattr_data *xattr_value = *xattr_value_;
>> +	int xattr_len = *xattr_len_, rc = xattr_len, hash_start = 0;
>> +	bool appraising_modsig = false;
>> +	void *xattr_value_evm;
>> +	size_t xattr_len_evm;
>> +
>> +	if (iint->flags & IMA_MODSIG_ALLOWED) {
>> +		/*
>> +		 * Not supposed to happen. Hooks that support modsig are
>> +		 * whitelisted when parsing the policy using
>> +		 * ima_hooks_supports_modsig.
>> +		 */
>> +		if (!buf || !size)
>> +			WARN_ONCE(true, "%s doesn't support modsig\n",
>> +				  func_tokens[func]);
>
> ima _appraise_measurement() is getting kind of long. Is there any
> reason we can't move this comment and test to ima_read_modsig()?

I didn't do that because then I would need to pass func as an argument
to ima_read_modsig just to print the warning above. But it does simplify
the code so it may be worth it. I'll make that change in v4.

>> @@ -229,8 +251,24 @@ int ima_appraise_measurement(enum ima_hooks func,
>>  		goto out;
>>  	}
>> 
>> -	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
>> -	if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
>> +	/*
>> +	 * Appended signatures aren't protected by EVM but we still call
>> +	 * evm_verifyxattr to check other security xattrs, if they exist.
>> +	 */
>> +	if (appraising_modsig) {
>> +		xattr_value_evm = NULL;
>> +		xattr_len_evm = 0;
>> +	} else {
>> +		xattr_value_evm = xattr_value;
>> +		xattr_len_evm = xattr_len;
>> +	}
>> +
>> +	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value_evm,
>> +				 xattr_len_evm, iint);
>> +	if (appraising_modsig && status == INTEGRITY_FAIL) {
>> +		cause = "invalid-HMAC";
>> +		goto out;
>
> "modsig" is special, because having any security xattrs is not
> required. This test doesn't prevent status from being set to
> "missing-HMAC". This test is redundant with the original tests below.

Indeed, that is wrong. I'm still a bit fuzzy about how EVM works and how
it interacts with IMA. The only way I can think of singling out modsig
without reintroduced the complex expression you didn't like in v2 is as
below. What do you think?

@@ -229,8 +241,25 @@ int ima_appraise_measurement(enum ima_hooks func,
 		goto out;
 	}
 
-	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
-	if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
+	/*
+	 * Appended signatures aren't protected by EVM but we still call
+	 * evm_verifyxattr to check other security xattrs, if they exist.
+	 */
+	if (appraising_modsig) {
+		xattr_value_evm = NULL;
+		xattr_len_evm = 0;
+	} else {
+		xattr_value_evm = xattr_value;
+		xattr_len_evm = xattr_len;
+	}
+
+	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value_evm,
+				 xattr_len_evm, iint);
+	if (appraising_modsig && (status == INTEGRITY_NOLABEL
+				  || status == INTEGRITY_NOXATTRS))
+		/* It's ok if there's no xattr in the case of modsig. */
+		;
+	else if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN) {
 		if ((status == INTEGRITY_NOLABEL)
 		    || (status == INTEGRITY_NOXATTRS))
 			cause = "missing-HMAC";

>> +	} else if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN) {
>>  		if ((status == INTEGRITY_NOLABEL)
>>  		    || (status == INTEGRITY_NOXATTRS))
>>  			cause = "missing-HMAC";
>> @@ -281,6 +319,43 @@ int ima_appraise_measurement(enum ima_hooks func,
>>  			status = INTEGRITY_PASS;
>>  		}
>
> Calling evm_verifyxattr() with the IMA xattr value prevents EVM from
> having to re-read the IMA xattr, but isn't necessary.On modsig
> signature verification failure, calling evm_verifyxattr() a second
> time isn't necessary.

So even for the IMA xattr sig case, the evm_verifyxattr call in
ima_appraise_measurement is an optimization and can be skipped?

>> +	case IMA_MODSIG:
>> +		/*
>> +		 * To avoid being tricked into recursion, we don't allow a
>> +		 * modsig stored in the xattr.
>> +		 */
>> +		if (!appraising_modsig) {
>> +			status = INTEGRITY_UNKNOWN;
>> +			cause = "unknown-ima-data";
>> +
>> +			break;
>> +		}
>> +
>> +		rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, xattr_value);
>> +		if (!rc) {
>> +			iint->flags |= IMA_DIGSIG;
>> +			status = 
>> +
>> +			kfree(*xattr_value_);
>> +			*xattr_value_ = xattr_value;
>> +			*xattr_len_ = xattr_len;
>> +
>> +			break;
>> +		}
>
> When including the appended signature in the measurement list, the
> corresponding file hash needs to be included in the measurement list,
> which might be different than the previously calculated file hash
> based on the hash algorithm as defined in the IMA xattr.
>
> Including the file hash and signature in the measurement list allows
> the attestation server, with just a public key, to verify the file
> signature against the file hash. No need for a white list.
>
> ima_modsig_verify() must calculate the file hash in order to verify
> the file signature. This file hash value somehow needs to be returned
> in order for it to be included in the measurement list.

In that case, patch 6/7 "ima: Store measurement after appraisal" isn't
enough and we have to go back to v2's change in ima_main.c which ties
together the collect and appraise steps in process_measurement (In that
version I called the function measure_and_appraise but it should be
called collect_and_appraise instead). That is because if the modsig
verification fails, the hash needs to be recalculated for the xattr
signature verification.

Either that, or I add another call to ima_collect_measurement inside
ima_appraise_measurement if the modsig verification fails. Which do you
prefer?

>> +		/*
>> +		 * The appended signature failed verification. Let's try
>> +		 * reading a signature from the extended attribute instead.
>> +		 */
>> +
>> +		pr_debug("modsig didn't verify, trying the xattr signature\n");
>> +
>> +		ima_free_xattr_data(xattr_value);
>> +		iint->flags &= ~IMA_MODSIG_ALLOWED;
>> +
>> +		return ima_appraise_measurement(func, iint, file, buf, size,
>> +						filename, xattr_value_,
>> +						xattr_len_, opened);
>
> Most of the code before "switch" needs to be done only once. Is
> recursion necessary? Or can we just retry the "switch" using the IMA
> xattr, assuming there is an IMA xattr?

I used recursion to avoid duplicating two blocks of code: the logic in
the "if (rc <= 0)" block which needs to be done again when verifying the
xattr sig, and also the logic of interpreting the return value of
evm_verifyxattr which I also thought needed to be done again, but you
seem to be saying that is just an optimization and can be skipped.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [Linux-ima-devel] [PATCH v3 1/7] integrity: Introduce struct evm_hmac_xattr

[Thiago J. B. <bau...@li...>]

Hello Mimi,

Thanks for your review!

The patch at the end of the email implements your suggestions, what do
you think?

Mimi Zohar <zo...@li...> writes:
> On Thu, 2017-07-06 at 19:17 -0300, Thiago Jung Bauermann wrote:
>> A separate struct evm_hmac_xattr is introduced, with the original
>> definition of evm_ima_xattr_data to be used in the places that actually
>> expect that definition.
>
> The new structure name implies that the xattr can only be an HMAC. By
> moving the new structure to evm.h we also loose the connection that it
> has to theevm_ima_xattr_type enumeration.

Ok, I renamed it to struct evm_xattr.

> Instead, how about defining the new struct in terms of the modified
> evm_ima_xattr_data struct?

IMHO IMA and EVM's practice of mixing and matching structs to compose
its data structures makes the code somewhat confusing and harder to see
where the actual storage used by a given field is actually allocated.

But I don't feel strongly about it, so the patch at the end of this
emails defines it as:

struct evm_xattr {
	struct evm_ima_xattr_data data;
	u8 digest[SHA1_DIGEST_SIZE];
} __packed;

Another disadvantage of the above is that you have two fields pointing
to the same place: evm_xattr.data.data and evm_xattr.digest.

> Please leave the new structure in integrity.h.

I think that moving the structure in evm.h is helpful. It immediately
conveys that nothing outside of security/integrity/evm/ interprets the
xattr data in the way defined by struct evm_xattr.

But I also don't feel strongly about it, so I moved it to integrity.h.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center


>From df2e29f29c99f5c986d8005d8af8409b3c4d115c Mon Sep 17 00:00:00 2001
From: Thiago Jung Bauermann <bau...@li...>
Date: Tue, 16 May 2017 17:14:49 -0300
Subject: [PATCH v4 1/7] integrity: Introduce struct evm_xattr

Even though struct evm_ima_xattr_data includes a fixed-size array to hold a
SHA1 digest, most of the code ignores the array and uses the struct to mean
"type indicator followed by data of unspecified size" and tracks the real
size of what the struct represents in a separate length variable.

The only exception to that is the EVM code, which correctly uses the
definition of struct evm_ima_xattr_data.

This patch makes this explicit in the code by removing the length
specification from the array in struct evm_ima_xattr_data. It also changes
the name of the element from digest to data, since in most places the array
doesn't hold a digest.

A separate struct evm_xattr is introduced, with the original definition of
evm_ima_xattr_data to be used in the places that actually expect that
definition.

Signed-off-by: Thiago Jung Bauermann <bau...@li...>
---
 security/integrity/evm/evm_crypto.c   |  4 ++--
 security/integrity/evm/evm_main.c     | 10 +++++-----
 security/integrity/ima/ima_appraise.c |  7 ++++---
 security/integrity/integrity.h        |  5 +++++
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index d7f282d75cc1..d902a18ef66f 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -252,13 +252,13 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 			const char *xattr_value, size_t xattr_value_len)
 {
 	struct inode *inode = d_backing_inode(dentry);
-	struct evm_ima_xattr_data xattr_data;
+	struct evm_xattr xattr_data;
 	int rc = 0;
 
 	rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
 			   xattr_value_len, xattr_data.digest);
 	if (rc == 0) {
-		xattr_data.type = EVM_XATTR_HMAC;
+		xattr_data.data.type = EVM_XATTR_HMAC;
 		rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
 					   &xattr_data,
 					   sizeof(xattr_data), 0);
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 063d38aef64e..536694499515 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -116,7 +116,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 					     struct integrity_iint_cache *iint)
 {
 	struct evm_ima_xattr_data *xattr_data = NULL;
-	struct evm_ima_xattr_data calc;
+	struct evm_xattr calc;
 	enum integrity_status evm_status = INTEGRITY_PASS;
 	int rc, xattr_len;
 
@@ -147,7 +147,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 	/* check value type */
 	switch (xattr_data->type) {
 	case EVM_XATTR_HMAC:
-		if (xattr_len != sizeof(struct evm_ima_xattr_data)) {
+		if (xattr_len != sizeof(struct evm_xattr)) {
 			evm_status = INTEGRITY_FAIL;
 			goto out;
 		}
@@ -155,7 +155,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 				   xattr_value_len, calc.digest);
 		if (rc)
 			break;
-		rc = crypto_memneq(xattr_data->digest, calc.digest,
+		rc = crypto_memneq(xattr_data->data, calc.digest,
 			    sizeof(calc.digest));
 		if (rc)
 			rc = -EINVAL;
@@ -467,7 +467,7 @@ int evm_inode_init_security(struct inode *inode,
 				 const struct xattr *lsm_xattr,
 				 struct xattr *evm_xattr)
 {
-	struct evm_ima_xattr_data *xattr_data;
+	struct evm_xattr *xattr_data;
 	int rc;
 
 	if (!evm_initialized || !evm_protected_xattr(lsm_xattr->name))
@@ -477,7 +477,7 @@ int evm_inode_init_security(struct inode *inode,
 	if (!xattr_data)
 		return -ENOMEM;
 
-	xattr_data->type = EVM_XATTR_HMAC;
+	xattr_data->data.type = EVM_XATTR_HMAC;
 	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
 	if (rc < 0)
 		goto out;
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 809ba70fbbbf..87d2b601cf8e 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -156,7 +156,8 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 		return sig->hash_algo;
 		break;
 	case IMA_XATTR_DIGEST_NG:
-		ret = xattr_value->digest[0];
+		/* first byte contains algorithm id */
+		ret = xattr_value->data[0];
 		if (ret < HASH_ALGO__LAST)
 			return ret;
 		break;
@@ -164,7 +165,7 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 		/* this is for backward compatibility */
 		if (xattr_len == 21) {
 			unsigned int zero = 0;
-			if (!memcmp(&xattr_value->digest[16], &zero, 4))
+			if (!memcmp(&xattr_value->data[16], &zero, 4))
 				return HASH_ALGO_MD5;
 			else
 				return HASH_ALGO_SHA1;
@@ -253,7 +254,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 			/* xattr length may be longer. md5 hash in previous
 			   version occupied 20 bytes in xattr, instead of 16
 			 */
-			rc = memcmp(&xattr_value->digest[hash_start],
+			rc = memcmp(&xattr_value->data[hash_start],
 				    iint->ima_hash->digest,
 				    iint->ima_hash->length);
 		else
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index a53e7e4ab06c..9b1762076f38 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -63,6 +63,11 @@ enum evm_ima_xattr_type {
 
 struct evm_ima_xattr_data {
 	u8 type;
+	u8 data[];
+} __packed;
+
+struct evm_xattr {
+	struct evm_ima_xattr_data data;
 	u8 digest[SHA1_DIGEST_SIZE];
 } __packed;
 
-- 
2.13.0

Re: [Linux-ima-devel] [PATCH v4 2/5] ima: use fs method to read integrity data [updated]

[Mimi Z. <zo...@li...>]

On Wed, 2017-08-02 at 10:01 +0200, Jan Kara wrote:
> On Tue 01-08-17 16:24:30, Mimi Zohar wrote:
> > From: Christoph Hellwig <hc...@ls...>
> > 
> > Add a new ->integrity_read file operation to read data for integrity
> > hash collection.  This is defined to be equivalent to ->read_iter,
> > except that it will be called with the i_rwsem held exclusively.
> > 
> > Signed-off-by: Christoph Hellwig <hc...@ls...>
> > Cc: Matthew Garrett <mat...@ne...>
> > Cc: Jan Kara <ja...@su...>
> > Cc: "Theodore Ts'o" <ty...@mi...>
> > Cc: Andreas Dilger <adi...@di...>
> > Cc: Jaegeuk Kim <ja...@ke...>
> > Cc: Chao Yu <yu...@hu...>
> > Cc: Steven Whitehouse <swh...@re...>
> > Cc: Bob Peterson <rpe...@re...>
> > Cc: David Woodhouse <dw...@in...>
> > Cc: Dave Kleikamp <sh...@ke...>
> > Cc: Ryusuke Konishi <kon...@la...>
> > Cc: Mark Fasheh <mf...@ve...>
> > Cc: Joel Becker <jl...@ev...>
> > Cc: Richard Weinberger <ri...@no...>
> > Cc: "Darrick J. Wong" <dar...@or...>
> > Cc: Hugh Dickins <hu...@go...>
> > Cc: Chris Mason <cl...@fb...>
> > Signed-off-by: Mimi Zohar <zo...@li...>
> 
> ...
> 
> > +static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb,
> > +					     struct iov_iter *to)
> > +{
> > +	struct inode *inode = file_inode(iocb->ki_filp);
> > +	int o_direct = iocb->ki_flags & IOCB_DIRECT;
> > +
> > +	lockdep_assert_held(&inode->i_rwsem);
> > +	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
> > +		return -EIO;
> > +
> > +	if (!iov_iter_count(to))
> > +		return 0; /* skip atime */
> > +
> > +#ifdef CONFIG_FS_DAX
> > +	if (IS_DAX(inode))
> > +		return dax_iomap_rw(iocb, to, &ext4_iomap_ops);
> > +#endif
> > +	if (o_direct)
> > +		return -EINVAL;
> > +	return generic_file_read_iter(iocb, to);
> > +}
> 
> I have noticed this o_direct check - why is it only in ext4 and shouldn't
> rather higher layers make sure IOCB_DIRECT iocbs cannot reach
> .integrity_read() methods?
 
This failure happens when opening a file with O_DIRECT on a block
device that does not support dax (eg. loop). xfs makes it to here too,
but the call to generic_file_read_iter() fails properly with -EINVAL. 
(Only tested on those filesystems included that support dax (eg. ext2,
ext4, and xfs).)

Mimi

Re: [Linux-ima-devel] [PATCH, RESEND 08/12] ima: added parser for RPM data type

[Roberto S. <rob...@hu...>]

On 8/2/2017 9:22 AM, James Morris wrote:
> On Tue, 1 Aug 2017, Roberto Sassu wrote:
>
>> On 8/1/2017 12:27 PM, Christoph Hellwig wrote:
>>> On Tue, Aug 01, 2017 at 12:20:36PM +0200, Roberto Sassu wrote:
>>>> This patch introduces a parser for RPM packages. It extracts the digests
>>>> from the RPMTAG_FILEDIGESTS header section and converts them to binary
>>>> data
>>>> before adding them to the hash table.
>>>>
>>>> The advantage of this data type is that verifiers can determine who
>>>> produced that data, as headers are signed by Linux distributions vendors.
>>>> RPM headers signatures can be provided as digest list metadata.
>>>
>>> Err, parsing arbitrary file formats has no business in the kernel.
>>
>> The benefit of this choice is that no actions are required for
>> Linux distribution vendors to support the solution I'm proposing,
>> because they already provide signed digest lists (RPM headers).
>>
>> Since the proof of loading a digest list is the digest of the
>> digest list (included in the list metadata), if RPM headers are
>> converted to a different format, remote attestation verifiers
>> cannot check the signature.
>>
>> If the concern is security, it would be possible to prevent unsigned
>> RPM headers from being parsed, if the PGP key type is upstreamed
>> (adding in CC key...@vg...).
>
> It's a security concern and also a layering violation, there should be no
> need to parse package file formats in the kernel.
>
> I'm not really clear on exactly how this patch series works.  Can you
> provide a more concrete explanation of what steps would occur during boot
> and attestation?

The main idea of this patch set is that, if a system executes
or reads good files (e.g. those from a Linux distribution),
the difference between the assertion 'a file could have possibly
been accessed' and 'a file has been accessed' is not relevant
for verifiers that only check the provenance of software.

Then, for those verifiers, a measurement representing the list of
good files which could have possibly been accessed gives the same
guarantees of individual file measurements.

The patch set introduces two data types:

- digest list: contains the digests of good files
- list metadata: contains the digest, the signature and the path
                  of each digest list to load (why loading many
                  lists instead of one will be clear after I explain
                  the remote attestation verification process)

Steps at boot:

1) systemd reads the path of list metadata from /etc/ima/digest-lists
    and writes it to a new securityfs file created by IMA
2) IMA reads and parses the list metadata (same mechanism for
    loading a policy, already upstreamed)
3) for each list metadata, IMA reads and parses the digest list
    and adds the file digests to a hash table
4) when a file is accessed, IMA calculates the digest as before
    and searches the file digest in the new hash table; if the
    digest is found, IMA sets the IMA_MEASURED flag in the inode
    metadata and clears the IMA_MEASURE action

Notes:

- list metadata and digest lists are measured before IMA reads them
- the digest of digest lists is also added to the hash table, otherwise
   there would be a measurement for each digest list

The measurement list looks like:

10 <template digest> ima-ng <digest> boot_aggregate
10 <template digest> ima-ng <digest> systemd (exe + libs)
10 <template digest> ima-ng <digest> /etc/ima/digest-lists
10 <template digest> ima-ng <digest> <list metadata>
10 <template digest> ima-ng <digest> <unknown files>


Steps during the verification:

Case 1: list metadata and digest lists are provided to verifiers

This is necessary when:
- digest lists are not signed
- verifiers do not trust the signer
- verifiers want to perform more checks on digest lists
   (digest lists may contain digest of outdated software)

Verifiers:

1) parse the list metadata received
2) for each digest list received, calculate the digest and
    compare it with the digest included in the list metadata
3) calculate the digest of list metadata and compare it with
    the digest in the measurement list
4) calculate the digest of the path of list metadata and compare
    it with the digest of /etc/ima/digest-lists in the measurement list
5) check boot_aggregate, systemd exe and libs, and unknown files
6) check the digest lists


Case 2: only list metadata is provided to verifiers

Verifiers:

1) parse the list metadata received
2) for each digest list, verify the signature
3) calculate the digest of the path of list metadata and compare
    it with the digest of /etc/ima/digest-lists in the measurement list
4) check boot_aggregate, systemd exe and libs, and unknown files

In Case 2, the verification process is simplified, because if
the signature of digest lists is valid, this means that possibly
accessed files are provided by the signer.

The problem here is that verifiers know the digest of possibly
accessed files from the measurement done by IMA at the time
digest lists are read. If IMA cannot parse the original (signed)
digest list, it would measure something that cannot be verified
with the signature.

RPM-based distributions already provide signed digest lists
(the RPM headers) for each package. To avoid the performance
penalty due to extending a PCR for each digest list, only
an entry for the list metadata is added to the measurement list.

For RPM-based Linux distributions, the full lifecycle for configuring
IMA can be implemented with very low effort. The tasks are:

1) systemd patch to load list metadata: just extend the existing
    patch to load the policy
2) userspace tools to parse the RPM database: done
3) dracut module to generate and include the digest lists and
    metadata into the initial ram disk: at the moment this is done
    from the dracut command line
4) plugin for the software management that executes the tool
    to generate the digest list for updated packages: to be implemented

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG