Menu
▾
▴
linux-ima-devel — Discussion list for problems or development.
This list is closed, nobody may subscribe to it.
| 2007 |
Jan
|
Feb
(1) |
Mar
|
Apr
|
May
|
Jun
|
Jul
|
Aug
|
Sep
|
Oct
|
Nov
|
Dec
(1) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 2009 |
Jan
|
Feb
|
Mar
|
Apr
(1) |
May
(1) |
Jun
(2) |
Jul
|
Aug
|
Sep
|
Oct
|
Nov
|
Dec
|
| 2011 |
Jan
|
Feb
|
Mar
(1) |
Apr
|
May
|
Jun
|
Jul
|
Aug
|
Sep
|
Oct
|
Nov
|
Dec
|
| 2013 |
Jan
|
Feb
|
Mar
(7) |
Apr
|
May
(7) |
Jun
(7) |
Jul
(26) |
Aug
|
Sep
(7) |
Oct
(1) |
Nov
(35) |
Dec
(18) |
| 2014 |
Jan
(1) |
Feb
(2) |
Mar
(3) |
Apr
|
May
(16) |
Jun
(35) |
Jul
(103) |
Aug
(45) |
Sep
(226) |
Oct
(200) |
Nov
(66) |
Dec
(42) |
| 2015 |
Jan
(47) |
Feb
(3) |
Mar
(6) |
Apr
(14) |
May
(38) |
Jun
(10) |
Jul
(10) |
Aug
(15) |
Sep
(23) |
Oct
(78) |
Nov
(56) |
Dec
(70) |
| 2016 |
Jan
(9) |
Feb
(8) |
Mar
(15) |
Apr
(18) |
May
(78) |
Jun
(39) |
Jul
(3) |
Aug
(136) |
Sep
(134) |
Oct
(19) |
Nov
(48) |
Dec
(30) |
| 2017 |
Jan
(33) |
Feb
(35) |
Mar
(100) |
Apr
(87) |
May
(169) |
Jun
(119) |
Jul
(165) |
Aug
(241) |
Sep
(128) |
Oct
(42) |
Nov
|
Dec
|
|
From: Gilad Ben-Y. <gi...@be...> - 2017-08-24 14:20:08
|
Now that -EBUSY return code only indicates backlog queueing we can safely remove the now redundant check for the CRYPTO_TFM_REQ_MAY_BACKLOG flag when -EBUSY is returned. Signed-off-by: Gilad Ben-Yossef <gi...@be...> --- crypto/ahash.c | 12 +++--------- crypto/cts.c | 6 ++---- crypto/lrw.c | 8 ++------ crypto/rsa-pkcs1pad.c | 16 ++++------------ crypto/xts.c | 8 ++------ 5 files changed, 13 insertions(+), 37 deletions(-) diff --git a/crypto/ahash.c b/crypto/ahash.c index 5e8666e..3a35d67 100644 --- a/crypto/ahash.c +++ b/crypto/ahash.c @@ -334,9 +334,7 @@ static int ahash_op_unaligned(struct ahash_request *req, return err; err = op(req); - if (err == -EINPROGRESS || - (err == -EBUSY && (ahash_request_flags(req) & - CRYPTO_TFM_REQ_MAY_BACKLOG))) + if (err == -EINPROGRESS || err == -EBUSY) return err; ahash_restore_req(req, err); @@ -394,9 +392,7 @@ static int ahash_def_finup_finish1(struct ahash_request *req, int err) req->base.complete = ahash_def_finup_done2; err = crypto_ahash_reqtfm(req)->final(req); - if (err == -EINPROGRESS || - (err == -EBUSY && (ahash_request_flags(req) & - CRYPTO_TFM_REQ_MAY_BACKLOG))) + if (err == -EINPROGRESS || err == -EBUSY) return err; out: @@ -432,9 +428,7 @@ static int ahash_def_finup(struct ahash_request *req) return err; err = tfm->update(req); - if (err == -EINPROGRESS || - (err == -EBUSY && (ahash_request_flags(req) & - CRYPTO_TFM_REQ_MAY_BACKLOG))) + if (err == -EINPROGRESS || err == -EBUSY) return err; return ahash_def_finup_finish1(req, err); diff --git a/crypto/cts.c b/crypto/cts.c index 243f591..4773c18 100644 --- a/crypto/cts.c +++ b/crypto/cts.c @@ -136,8 +136,7 @@ static void crypto_cts_encrypt_done(struct crypto_async_request *areq, int err) goto out; err = cts_cbc_encrypt(req); - if (err == -EINPROGRESS || - (err == -EBUSY && req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) + if (err == -EINPROGRESS || err == -EBUSY) return; out: @@ -229,8 +228,7 @@ static void crypto_cts_decrypt_done(struct crypto_async_request *areq, int err) goto out; err = cts_cbc_decrypt(req); - if (err == -EINPROGRESS || - (err == -EBUSY && req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) + if (err == -EINPROGRESS || err == -EBUSY) return; out: diff --git a/crypto/lrw.c b/crypto/lrw.c index a8bfae4..695cea9 100644 --- a/crypto/lrw.c +++ b/crypto/lrw.c @@ -328,9 +328,7 @@ static int do_encrypt(struct skcipher_request *req, int err) crypto_skcipher_encrypt(subreq) ?: post_crypt(req); - if (err == -EINPROGRESS || - (err == -EBUSY && - req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) + if (err == -EINPROGRESS || err == -EBUSY) return err; } @@ -380,9 +378,7 @@ static int do_decrypt(struct skcipher_request *req, int err) crypto_skcipher_decrypt(subreq) ?: post_crypt(req); - if (err == -EINPROGRESS || - (err == -EBUSY && - req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) + if (err == -EINPROGRESS || err == -EBUSY) return err; } diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c index 407c64b..2908f93 100644 --- a/crypto/rsa-pkcs1pad.c +++ b/crypto/rsa-pkcs1pad.c @@ -279,9 +279,7 @@ static int pkcs1pad_encrypt(struct akcipher_request *req) req->dst, ctx->key_size - 1, req->dst_len); err = crypto_akcipher_encrypt(&req_ctx->child_req); - if (err != -EINPROGRESS && - (err != -EBUSY || - !(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG))) + if (err != -EINPROGRESS && err != -EBUSY) return pkcs1pad_encrypt_sign_complete(req, err); return err; @@ -383,9 +381,7 @@ static int pkcs1pad_decrypt(struct akcipher_request *req) ctx->key_size); err = crypto_akcipher_decrypt(&req_ctx->child_req); - if (err != -EINPROGRESS && - (err != -EBUSY || - !(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG))) + if (err != -EINPROGRESS && err != -EBUSY) return pkcs1pad_decrypt_complete(req, err); return err; @@ -440,9 +436,7 @@ static int pkcs1pad_sign(struct akcipher_request *req) req->dst, ctx->key_size - 1, req->dst_len); err = crypto_akcipher_sign(&req_ctx->child_req); - if (err != -EINPROGRESS && - (err != -EBUSY || - !(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG))) + if (err != -EINPROGRESS && err != -EBUSY) return pkcs1pad_encrypt_sign_complete(req, err); return err; @@ -561,9 +555,7 @@ static int pkcs1pad_verify(struct akcipher_request *req) ctx->key_size); err = crypto_akcipher_verify(&req_ctx->child_req); - if (err != -EINPROGRESS && - (err != -EBUSY || - !(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG))) + if (err != -EINPROGRESS && err != -EBUSY) return pkcs1pad_verify_complete(req, err); return err; diff --git a/crypto/xts.c b/crypto/xts.c index d86c11a..af68012 100644 --- a/crypto/xts.c +++ b/crypto/xts.c @@ -269,9 +269,7 @@ static int do_encrypt(struct skcipher_request *req, int err) crypto_skcipher_encrypt(subreq) ?: post_crypt(req); - if (err == -EINPROGRESS || - (err == -EBUSY && - req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) + if (err == -EINPROGRESS || err == -EBUSY) return err; } @@ -321,9 +319,7 @@ static int do_decrypt(struct skcipher_request *req, int err) crypto_skcipher_decrypt(subreq) ?: post_crypt(req); - if (err == -EINPROGRESS || - (err == -EBUSY && - req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) + if (err == -EINPROGRESS || err == -EBUSY) return err; } -- 2.1.4 |
|
From: Gilad Ben-Y. <gi...@be...> - 2017-08-24 14:19:56
|
Replace -EBUSY with -EAGAIN when reporting transient busy
indication in the absence of backlog.
Signed-off-by: Gilad Ben-Yossef <gi...@be...>
Reviewed-by: Gary R Hook <gar...@am...>
---
drivers/crypto/ccp/ccp-crypto-main.c | 8 +++-----
drivers/crypto/ccp/ccp-dev.c | 7 +++++--
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/crypto/ccp/ccp-crypto-main.c b/drivers/crypto/ccp/ccp-crypto-main.c
index 35a9de7..403ff0a 100644
--- a/drivers/crypto/ccp/ccp-crypto-main.c
+++ b/drivers/crypto/ccp/ccp-crypto-main.c
@@ -222,9 +222,10 @@ static int ccp_crypto_enqueue_cmd(struct ccp_crypto_cmd *crypto_cmd)
/* Check if the cmd can/should be queued */
if (req_queue.cmd_count >= CCP_CRYPTO_MAX_QLEN) {
- ret = -EBUSY;
- if (!(crypto_cmd->cmd->flags & CCP_CMD_MAY_BACKLOG))
+ if (!(crypto_cmd->cmd->flags & CCP_CMD_MAY_BACKLOG)) {
+ ret = -EAGAIN;
goto e_lock;
+ }
}
/* Look for an entry with the same tfm. If there is a cmd
@@ -243,9 +244,6 @@ static int ccp_crypto_enqueue_cmd(struct ccp_crypto_cmd *crypto_cmd)
ret = ccp_enqueue_cmd(crypto_cmd->cmd);
if (!ccp_crypto_success(ret))
goto e_lock; /* Error, don't queue it */
- if ((ret == -EBUSY) &&
- !(crypto_cmd->cmd->flags & CCP_CMD_MAY_BACKLOG))
- goto e_lock; /* Not backlogging, don't queue it */
}
if (req_queue.cmd_count >= CCP_CRYPTO_MAX_QLEN) {
diff --git a/drivers/crypto/ccp/ccp-dev.c b/drivers/crypto/ccp/ccp-dev.c
index 4e029b1..3d637e3 100644
--- a/drivers/crypto/ccp/ccp-dev.c
+++ b/drivers/crypto/ccp/ccp-dev.c
@@ -292,9 +292,12 @@ int ccp_enqueue_cmd(struct ccp_cmd *cmd)
i = ccp->cmd_q_count;
if (ccp->cmd_count >= MAX_CMD_QLEN) {
- ret = -EBUSY;
- if (cmd->flags & CCP_CMD_MAY_BACKLOG)
+ if (cmd->flags & CCP_CMD_MAY_BACKLOG) {
+ ret = -EBUSY;
list_add_tail(&cmd->entry, &ccp->backlog);
+ } else {
+ ret = -EAGAIN;
+ }
} else {
ret = -EINPROGRESS;
ccp->cmd_count++;
--
2.1.4
|
|
From: Gilad Ben-Y. <gi...@be...> - 2017-08-24 14:19:43
|
The crypto API was using the -EBUSY return value to indicate
both a hard failure to submit a crypto operation into a
transformation provider when the latter was busy and the backlog
mechanism was not enabled as well as a notification that the
operation was queued into the backlog when the backlog mechanism
was enabled.
Having the same return code indicate two very different conditions
depending on a flag is both error prone and requires extra runtime
check like the following to discern between the cases:
if (err == -EINPROGRESS ||
(err == -EBUSY && (ahash_request_flags(req) &
CRYPTO_TFM_REQ_MAY_BACKLOG)))
This patch changes the return code used to indicate a crypto op
failed due to the transformation provider being transiently busy
to -EAGAIN.
Signed-off-by: Gilad Ben-Yossef <gi...@be...>
---
crypto/algapi.c | 6 ++++--
crypto/algif_hash.c | 20 +++++++++++++++++---
crypto/cryptd.c | 4 +---
3 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/crypto/algapi.c b/crypto/algapi.c
index aa699ff..916bee3 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -897,9 +897,11 @@ int crypto_enqueue_request(struct crypto_queue *queue,
int err = -EINPROGRESS;
if (unlikely(queue->qlen >= queue->max_qlen)) {
- err = -EBUSY;
- if (!(request->flags & CRYPTO_TFM_REQ_MAY_BACKLOG))
+ if (!(request->flags & CRYPTO_TFM_REQ_MAY_BACKLOG)) {
+ err = -EAGAIN;
goto out;
+ }
+ err = -EBUSY;
if (queue->backlog == &queue->list)
queue->backlog = &request->list;
}
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index 5e92bd2..3b3c154 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -39,6 +39,20 @@ struct algif_hash_tfm {
bool has_key;
};
+/* Previous versions of crypto_* ops used to return -EBUSY
+ * rather than -EAGAIN to indicate being tied up. The in
+ * kernel API changed but we don't want to break the user
+ * space API. As only the hash user interface exposed this
+ * error ever to the user, do the translation here.
+ */
+static inline int crypto_user_err(int err)
+{
+ if (err == -EAGAIN)
+ return -EBUSY;
+
+ return err;
+}
+
static int hash_alloc_result(struct sock *sk, struct hash_ctx *ctx)
{
unsigned ds;
@@ -136,7 +150,7 @@ static int hash_sendmsg(struct socket *sock, struct msghdr *msg,
unlock:
release_sock(sk);
- return err ?: copied;
+ return err ? crypto_user_err(err) : copied;
}
static ssize_t hash_sendpage(struct socket *sock, struct page *page,
@@ -188,7 +202,7 @@ static ssize_t hash_sendpage(struct socket *sock, struct page *page,
unlock:
release_sock(sk);
- return err ?: size;
+ return err ? crypto_user_err(err) : size;
}
static int hash_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
@@ -236,7 +250,7 @@ static int hash_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
hash_free_result(sk, ctx);
release_sock(sk);
- return err ?: len;
+ return err ? crypto_user_err(err) : len;
}
static int hash_accept(struct socket *sock, struct socket *newsock, int flags,
diff --git a/crypto/cryptd.c b/crypto/cryptd.c
index 0508c48..d1dbdce 100644
--- a/crypto/cryptd.c
+++ b/crypto/cryptd.c
@@ -137,16 +137,14 @@ static int cryptd_enqueue_request(struct cryptd_queue *queue,
int cpu, err;
struct cryptd_cpu_queue *cpu_queue;
atomic_t *refcnt;
- bool may_backlog;
cpu = get_cpu();
cpu_queue = this_cpu_ptr(queue->cpu_queue);
err = crypto_enqueue_request(&cpu_queue->queue, request);
refcnt = crypto_tfm_ctx(request->tfm);
- may_backlog = request->flags & CRYPTO_TFM_REQ_MAY_BACKLOG;
- if (err == -EBUSY && !may_backlog)
+ if (err == -EAGAIN)
goto out_put_cpu;
queue_work_on(cpu, kcrypto_wq, &cpu_queue->work);
--
2.1.4
|
|
From: Gilad Ben-Y. <gi...@be...> - 2017-08-24 14:19:31
|
Many users of kernel async. crypto services have a pattern of starting an async. crypto op and than using a completion to wait for it to end. This patch set simplifies this common use case in two ways: First, by separating the return codes of the case where a request is queued to a backlog due to the provider being busy (-EBUSY) from the case the request has failed due to the provider being busy and backlogging is not enabled (-EAGAIN). Next, this change is than built on to create a generic API to wait for a async. crypto operation to complete. The end result is a smaller code base and an API that is easier to use and more difficult to get wrong. The patch set was boot tested on x86_64 and arm64 which at the very least tests the crypto users via testmgr and tcrypt but I do note that I do not have access to some of the HW whose drivers are modified nor do I claim I was able to test all of the corner cases. The patch set is based upon linux-next release tagged next-20170824. Changes from v6: - Fix brown paper bag compile error on marvell/cesa code. Changes from v5: - Remove redundant new line as spotted by Jonathan Cameron. - Reworded dm-verity change commit message to better clarify potential issue averted by change as pointed out by Mikulas Patocka. Changes from v4: - Rebase on top of latest algif changes from Stephan Mueller. - Fix typo in ccp patch title. Changes from v3: - Instead of changing the return code to indicate backlog queueing, change the return code to indicate transient busy state, as suggested by Herbert Xu. Changes from v2: - Patch title changed from "introduce crypto wait for async op" to better reflect the current state. - Rebase on top of latest linux-next. - Add a new return code of -EIOCBQUEUED for backlog queueing, as suggested by Herbert Xu. - Transform more users to the new API. - Update the drbg change to account for new init as indicated by Stephan Muller. Changes from v1: - Address review comments from Eric Biggers. - Separated out bug fixes of existing code and rebase on top of that patch set. - Rename 'ecr' to 'wait' in fscrypto code. - Split patch introducing the new API from the change moving over the algif code which it originated from to the new API. - Inline crypto_wait_req(). - Some code indentation fixes. Gilad Ben-Yossef (19): crypto: change transient busy return code to -EAGAIN crypto: ccp: use -EAGAIN for transient busy indication crypto: remove redundant backlog checks on EBUSY crypto: marvell/cesa: remove redundant backlog checks on EBUSY crypto: introduce crypto wait for async op crypto: move algif to generic async completion crypto: move pub key to generic async completion crypto: move drbg to generic async completion crypto: move gcm to generic async completion crypto: move testmgr to generic async completion fscrypt: move to generic async completion dm: move dm-verity to generic async completion cifs: move to generic async completion ima: move to generic async completion crypto: tcrypt: move to generic async completion crypto: talitos: move to generic async completion crypto: qce: move to generic async completion crypto: mediatek: move to generic async completion crypto: adapt api sample to use async. op wait Documentation/crypto/api-samples.rst | 52 ++------- crypto/af_alg.c | 27 ----- crypto/ahash.c | 12 +-- crypto/algapi.c | 6 +- crypto/algif_aead.c | 8 +- crypto/algif_hash.c | 50 +++++---- crypto/algif_skcipher.c | 9 +- crypto/api.c | 13 +++ crypto/asymmetric_keys/public_key.c | 28 +---- crypto/cryptd.c | 4 +- crypto/cts.c | 6 +- crypto/drbg.c | 36 ++----- crypto/gcm.c | 32 ++---- crypto/lrw.c | 8 +- crypto/rsa-pkcs1pad.c | 16 +-- crypto/tcrypt.c | 84 +++++---------- crypto/testmgr.c | 204 ++++++++++++----------------------- crypto/xts.c | 8 +- drivers/crypto/ccp/ccp-crypto-main.c | 8 +- drivers/crypto/ccp/ccp-dev.c | 7 +- drivers/crypto/marvell/cesa.c | 3 +- drivers/crypto/marvell/cesa.h | 2 +- drivers/crypto/mediatek/mtk-aes.c | 31 +----- drivers/crypto/qce/sha.c | 30 +----- drivers/crypto/talitos.c | 38 +------ drivers/md/dm-verity-target.c | 81 ++++---------- drivers/md/dm-verity.h | 5 - fs/cifs/smb2ops.c | 30 +----- fs/crypto/crypto.c | 28 +---- fs/crypto/fname.c | 36 ++----- fs/crypto/fscrypt_private.h | 10 -- fs/crypto/keyinfo.c | 21 +--- include/crypto/drbg.h | 3 +- include/crypto/if_alg.h | 15 +-- include/linux/crypto.h | 40 +++++++ security/integrity/ima/ima_crypto.c | 56 +++------- 36 files changed, 310 insertions(+), 737 deletions(-) -- 2.1.4 |
|
From: Gilad Ben-Y. <gi...@be...> - 2017-08-24 13:53:18
|
On Thu, Aug 24, 2017 at 4:01 PM, kbuild test robot <lk...@in...> wrote: > Hi Gilad, > > [auto build test ERROR on cryptodev/master] > [also build test ERROR on v4.13-rc6 next-20170823] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Gilad-Ben-Yossef/crypto-change-transient-busy-return-code-to-EAGAIN/20170824-180606 > base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master > config: arm-allmodconfig (attached as .config) > compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705 > reproduce: > wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > make.cross ARCH=arm > > All error/warnings (new ones prefixed by >>): > > drivers/crypto/marvell/cesa.c: In function 'mv_cesa_queue_req': >>> drivers/crypto/marvell/cesa.c:187:3: error: expected ')' before 'mv_cesa_tdma_chain' > mv_cesa_tdma_chain(engine, creq); > ^~~~~~~~~~~~~~~~~~ >>> drivers/crypto/marvell/cesa.c:196:1: error: expected expression before '}' token > } > ^ >>> drivers/crypto/marvell/cesa.c:196:1: warning: control reaches end of non-void function [-Wreturn-type] > } > ^ Oy! I thought I've added COMPILE_TEST to my local tree to build it. I didn't. Fix underway. Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru |
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 13:41:16
|
Hi,
Yes, please add one of appropriate.
Thanks,
Dmitry
On Tue, Aug 22, 2017 at 4:13 PM, Mimi Zohar <zo...@li...> wrote:
> On Tue, 2017-08-22 at 13:07 +0300, Dmitry Kasatkin wrote:
>> Looks good to me.
>
> Thank you for reviewing the code! Can I add your Reviewed-by/Acked-by?
>
> Mimi
>
>>
>> On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
>> > Permit normally denied access/execute permission for files in policy
>> > on IMA unsupported filesystems. This patch defines "fs_unsafe", a
>> > builtin policy.
>> >
>> > Signed-off-by: Mimi Zohar <zo...@li...>
>> >
>> > ---
>> > Changelog v3:
>> > - include dont_failsafe rule when displaying policy
>> >
>> > Documentation/admin-guide/kernel-parameters.txt | 8 +++++++-
>> > security/integrity/ima/ima_policy.c | 12 ++++++++++++
>> > 2 files changed, 19 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> > index d9c171ce4190..4e303be83df6 100644
>> > --- a/Documentation/admin-guide/kernel-parameters.txt
>> > +++ b/Documentation/admin-guide/kernel-parameters.txt
>> > @@ -1502,7 +1502,7 @@
>> >
>> > ima_policy= [IMA]
>> > The builtin policies to load during IMA setup.
>> > - Format: "tcb | appraise_tcb | secure_boot"
>> > + Format: "tcb | appraise_tcb | secure_boot | fs_unsafe"
>> >
>> > The "tcb" policy measures all programs exec'd, files
>> > mmap'd for exec, and all files opened with the read
>> > @@ -1517,6 +1517,12 @@
>> > of files (eg. kexec kernel image, kernel modules,
>> > firmware, policy, etc) based on file signatures.
>> >
>> > + The "fs_unsafe" policy permits normally denied
>> > + access/execute permission for files in policy on IMA
>> > + unsupported filesystems. Note this option, as the
>> > + name implies, is not safe and not recommended for
>> > + any environments other than testing.
>> > +
>> > ima_tcb [IMA] Deprecated. Use ima_policy= instead.
>> > Load a policy which meets the needs of the Trusted
>> > Computing Base. This means IMA will measure all
>> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> > index 43b85a4fb8e8..cddd9dfb01e1 100644
>> > --- a/security/integrity/ima/ima_policy.c
>> > +++ b/security/integrity/ima/ima_policy.c
>> > @@ -169,6 +169,10 @@ static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
>> > .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
>> > };
>> >
>> > +static struct ima_rule_entry dont_failsafe_rules[] __ro_after_init = {
>> > + {.action = DONT_FAILSAFE}
>> > +};
>> > +
>> > static LIST_HEAD(ima_default_rules);
>> > static LIST_HEAD(ima_policy_rules);
>> > static LIST_HEAD(ima_temp_rules);
>> > @@ -188,6 +192,7 @@ __setup("ima_tcb", default_measure_policy_setup);
>> >
>> > static bool ima_use_appraise_tcb __initdata;
>> > static bool ima_use_secure_boot __initdata;
>> > +static bool ima_use_dont_failsafe __initdata;
>> > static int __init policy_setup(char *str)
>> > {
>> > char *p;
>> > @@ -201,6 +206,10 @@ static int __init policy_setup(char *str)
>> > ima_use_appraise_tcb = 1;
>> > else if (strcmp(p, "secure_boot") == 0)
>> > ima_use_secure_boot = 1;
>> > + else if (strcmp(p, "fs_unsafe") == 0) {
>> > + ima_use_dont_failsafe = 1;
>> > + set_failsafe(0);
>> > + }
>> > }
>> >
>> > return 1;
>> > @@ -470,6 +479,9 @@ void __init ima_init_policy(void)
>> > temp_ima_appraise |= IMA_APPRAISE_POLICY;
>> > }
>> >
>> > + if (ima_use_dont_failsafe)
>> > + list_add_tail(&dont_failsafe_rules[0].list, &ima_default_rules);
>> > +
>> > ima_rules = &ima_default_rules;
>> > ima_update_policy_flag();
>> > }
>> > --
>> > 2.7.4
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
>> > the body of a message to maj...@vg...
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>
--
Thanks,
Dmitry
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 13:31:14
|
On Tue, Aug 22, 2017 at 3:54 PM, Mimi Zohar <zo...@li...> wrote:
> On Tue, 2017-08-22 at 13:07 +0300, Dmitry Kasatkin wrote:
>> On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
>> > Permit normally denied access/execute permission for files in policy
>> > on IMA unsupported filesystems. This patch defines the "dont_failsafe"
>> > policy action rule.
>> >
>> > Signed-off-by: Mimi Zohar <zo...@li...>
>> >
>> > ---
>> > Changelog v3:
>> > - include dont_failsafe rule when displaying policy
>> > - fail attempt to add dont_failsafe rule when appending to the policy
>> >
>> > Documentation/ABI/testing/ima_policy | 3 ++-
>> > security/integrity/ima/ima.h | 1 +
>> > security/integrity/ima/ima_main.c | 12 +++++++++++-
>> > security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++++++-
>> > 4 files changed, 42 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>> > index e76432b9954d..f271207743e5 100644
>> > --- a/Documentation/ABI/testing/ima_policy
>> > +++ b/Documentation/ABI/testing/ima_policy
>> > @@ -17,7 +17,8 @@ Description:
>> >
>> > rule format: action [condition ...]
>> >
>> > - action: measure | dont_measure | appraise | dont_appraise | audit
>> > + action: measure | dont_meaure | appraise | dont_appraise |
>> > + audit | dont_failsafe
>> > condition:= base | lsm [option]
>> > base: [[func=] [mask=] [fsmagic=] [fsuuid=] [uid=]
>> > [euid=] [fowner=]]
>> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> > index d52b487ad259..c5f34f7c5b0f 100644
>> > --- a/security/integrity/ima/ima.h
>> > +++ b/security/integrity/ima/ima.h
>> > @@ -224,6 +224,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos);
>> > void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
>> > void ima_policy_stop(struct seq_file *m, void *v);
>> > int ima_policy_show(struct seq_file *m, void *v);
>> > +void set_failsafe(bool flag);
>> >
>> > /* Appraise integrity measurements */
>> > #define IMA_APPRAISE_ENFORCE 0x01
>> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> > index d23dfe6ede18..b00186914df8 100644
>> > --- a/security/integrity/ima/ima_main.c
>> > +++ b/security/integrity/ima/ima_main.c
>> > @@ -38,6 +38,12 @@ int ima_appraise;
>> > int ima_hash_algo = HASH_ALGO_SHA1;
>> > static int hash_setup_done;
>> >
>> > +static bool ima_failsafe = 1;
>> > +void set_failsafe(bool flag)
>> > +{
>> > + ima_failsafe = flag;
>> > +}
>> > +
>> > static int __init hash_setup(char *str)
>> > {
>> > struct ima_template_desc *template_desc = ima_template_desc_current();
>> > @@ -260,8 +266,12 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>> > __putname(pathbuf);
>> > out:
>> > inode_unlock(inode);
>> > - if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
>> > + if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
>> > + if (!ima_failsafe && rc == -EBADF)
>> > + return 0;
>> > +
>>
>> By default IMA is failsafe. ima_failsafe is true.
>> Return 0 is needed in failsafe mode. right?
>> But in this logic it will happen if ima_failsafe is false. meaning it
>> is not failsafe.
>>
>> Is it a typo?
>
> No, the default, as you pointed out above, is failsafe mode. Only when we are not in failsafe mode, do we allow the file access/execute for file's that we could not appraise.
>
> Mimi
>
So in your language "failsafe" means IMA must fail/return with error
on failure..
Ok. then logic is correct and OK with me.
--
Thanks,
Dmitry
|
|
From: Mimi Z. <zo...@li...> - 2017-08-22 13:14:03
|
On Tue, 2017-08-22 at 13:07 +0300, Dmitry Kasatkin wrote:
> Looks good to me.
Thank you for reviewing the code! Can I add your Reviewed-by/Acked-by?
Mimi
>
> On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
> > Permit normally denied access/execute permission for files in policy
> > on IMA unsupported filesystems. This patch defines "fs_unsafe", a
> > builtin policy.
> >
> > Signed-off-by: Mimi Zohar <zo...@li...>
> >
> > ---
> > Changelog v3:
> > - include dont_failsafe rule when displaying policy
> >
> > Documentation/admin-guide/kernel-parameters.txt | 8 +++++++-
> > security/integrity/ima/ima_policy.c | 12 ++++++++++++
> > 2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index d9c171ce4190..4e303be83df6 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -1502,7 +1502,7 @@
> >
> > ima_policy= [IMA]
> > The builtin policies to load during IMA setup.
> > - Format: "tcb | appraise_tcb | secure_boot"
> > + Format: "tcb | appraise_tcb | secure_boot | fs_unsafe"
> >
> > The "tcb" policy measures all programs exec'd, files
> > mmap'd for exec, and all files opened with the read
> > @@ -1517,6 +1517,12 @@
> > of files (eg. kexec kernel image, kernel modules,
> > firmware, policy, etc) based on file signatures.
> >
> > + The "fs_unsafe" policy permits normally denied
> > + access/execute permission for files in policy on IMA
> > + unsupported filesystems. Note this option, as the
> > + name implies, is not safe and not recommended for
> > + any environments other than testing.
> > +
> > ima_tcb [IMA] Deprecated. Use ima_policy= instead.
> > Load a policy which meets the needs of the Trusted
> > Computing Base. This means IMA will measure all
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 43b85a4fb8e8..cddd9dfb01e1 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -169,6 +169,10 @@ static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
> > .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
> > };
> >
> > +static struct ima_rule_entry dont_failsafe_rules[] __ro_after_init = {
> > + {.action = DONT_FAILSAFE}
> > +};
> > +
> > static LIST_HEAD(ima_default_rules);
> > static LIST_HEAD(ima_policy_rules);
> > static LIST_HEAD(ima_temp_rules);
> > @@ -188,6 +192,7 @@ __setup("ima_tcb", default_measure_policy_setup);
> >
> > static bool ima_use_appraise_tcb __initdata;
> > static bool ima_use_secure_boot __initdata;
> > +static bool ima_use_dont_failsafe __initdata;
> > static int __init policy_setup(char *str)
> > {
> > char *p;
> > @@ -201,6 +206,10 @@ static int __init policy_setup(char *str)
> > ima_use_appraise_tcb = 1;
> > else if (strcmp(p, "secure_boot") == 0)
> > ima_use_secure_boot = 1;
> > + else if (strcmp(p, "fs_unsafe") == 0) {
> > + ima_use_dont_failsafe = 1;
> > + set_failsafe(0);
> > + }
> > }
> >
> > return 1;
> > @@ -470,6 +479,9 @@ void __init ima_init_policy(void)
> > temp_ima_appraise |= IMA_APPRAISE_POLICY;
> > }
> >
> > + if (ima_use_dont_failsafe)
> > + list_add_tail(&dont_failsafe_rules[0].list, &ima_default_rules);
> > +
> > ima_rules = &ima_default_rules;
> > ima_update_policy_flag();
> > }
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> > the body of a message to maj...@vg...
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
|
|
From: Mimi Z. <zo...@li...> - 2017-08-22 12:55:11
|
On Tue, 2017-08-22 at 13:07 +0300, Dmitry Kasatkin wrote:
> On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
> > Permit normally denied access/execute permission for files in policy
> > on IMA unsupported filesystems. This patch defines the "dont_failsafe"
> > policy action rule.
> >
> > Signed-off-by: Mimi Zohar <zo...@li...>
> >
> > ---
> > Changelog v3:
> > - include dont_failsafe rule when displaying policy
> > - fail attempt to add dont_failsafe rule when appending to the policy
> >
> > Documentation/ABI/testing/ima_policy | 3 ++-
> > security/integrity/ima/ima.h | 1 +
> > security/integrity/ima/ima_main.c | 12 +++++++++++-
> > security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++++++-
> > 4 files changed, 42 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > index e76432b9954d..f271207743e5 100644
> > --- a/Documentation/ABI/testing/ima_policy
> > +++ b/Documentation/ABI/testing/ima_policy
> > @@ -17,7 +17,8 @@ Description:
> >
> > rule format: action [condition ...]
> >
> > - action: measure | dont_measure | appraise | dont_appraise | audit
> > + action: measure | dont_meaure | appraise | dont_appraise |
> > + audit | dont_failsafe
> > condition:= base | lsm [option]
> > base: [[func=] [mask=] [fsmagic=] [fsuuid=] [uid=]
> > [euid=] [fowner=]]
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index d52b487ad259..c5f34f7c5b0f 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -224,6 +224,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos);
> > void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> > void ima_policy_stop(struct seq_file *m, void *v);
> > int ima_policy_show(struct seq_file *m, void *v);
> > +void set_failsafe(bool flag);
> >
> > /* Appraise integrity measurements */
> > #define IMA_APPRAISE_ENFORCE 0x01
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index d23dfe6ede18..b00186914df8 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -38,6 +38,12 @@ int ima_appraise;
> > int ima_hash_algo = HASH_ALGO_SHA1;
> > static int hash_setup_done;
> >
> > +static bool ima_failsafe = 1;
> > +void set_failsafe(bool flag)
> > +{
> > + ima_failsafe = flag;
> > +}
> > +
> > static int __init hash_setup(char *str)
> > {
> > struct ima_template_desc *template_desc = ima_template_desc_current();
> > @@ -260,8 +266,12 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> > __putname(pathbuf);
> > out:
> > inode_unlock(inode);
> > - if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
> > + if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> > + if (!ima_failsafe && rc == -EBADF)
> > + return 0;
> > +
>
> By default IMA is failsafe. ima_failsafe is true.
> Return 0 is needed in failsafe mode. right?
> But in this logic it will happen if ima_failsafe is false. meaning it
> is not failsafe.
>
> Is it a typo?
No, the default, as you pointed out above, is failsafe mode. Only when we are not in failsafe mode, do we allow the file access/execute for file's that we could not appraise.
Mimi
|
|
From: Mimi Z. <zo...@li...> - 2017-08-22 12:54:42
|
On Tue, 2017-08-22 at 13:05 +0300, Dmitry Kasatkin wrote:
> On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
> > All files matching a "measure" rule must be included in the IMA
> > measurement list, even when the file hash cannot be calculated.
> > Similarly, all files matching an "audit" rule must be audited, even when
> > the file hash can not be calculated.
> >
> > The file data hash field contained in the IMA measurement list template
> > data will contain 0's instead of the actual file hash digest.
> >
> > Signed-off-by: Mimi Zohar <zo...@li...>
> >
> > ---
> > Changelog v6:
> > - replace "?:" with if/then
> > - annotate i_version usage
> > - reword O_DIRECT comment
> >
> > Changelog v5:
> > - Fail files opened O_DIRECT, but include attempt in measurement list.
> >
> > Changelog v4:
> > - Based on both -EBADF and -EINVAL
> > - clean up ima_collect_measurement()
> >
> > security/integrity/ima/ima_api.c | 67 +++++++++++++++++++++++--------------
> > security/integrity/ima/ima_crypto.c | 10 ++++++
> > security/integrity/ima/ima_main.c | 7 ++--
> > 3 files changed, 54 insertions(+), 30 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > index c2edba8de35e..1dee695642a4 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -199,42 +199,59 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> > struct inode *inode = file_inode(file);
> > const char *filename = file->f_path.dentry->d_name.name;
> > int result = 0;
> > + int length;
> > + void *tmpbuf;
> > + u64 i_version;
> > struct {
> > struct ima_digest_data hdr;
> > char digest[IMA_MAX_DIGEST_SIZE];
> > } hash;
> >
> > - if (!(iint->flags & IMA_COLLECTED)) {
> > - u64 i_version = file_inode(file)->i_version;
> > + if (iint->flags & IMA_COLLECTED)
> > + goto out;
> >
> > - if (file->f_flags & O_DIRECT) {
> > - audit_cause = "failed(directio)";
> > - result = -EACCES;
> > - goto out;
> > - }
> > + /*
> > + * Dectecting file change is based on i_version. On filesystems
> > + * which do not support i_version, support is limited to an initial
> > + * measurement/appraisal/audit.
> > + */
> > + i_version = file_inode(file)->i_version;
> > + hash.hdr.algo = algo;
> >
> > - hash.hdr.algo = algo;
> > -
> > - result = (!buf) ? ima_calc_file_hash(file, &hash.hdr) :
> > - ima_calc_buffer_hash(buf, size, &hash.hdr);
> > - if (!result) {
> > - int length = sizeof(hash.hdr) + hash.hdr.length;
> > - void *tmpbuf = krealloc(iint->ima_hash, length,
> > - GFP_NOFS);
> > - if (tmpbuf) {
> > - iint->ima_hash = tmpbuf;
> > - memcpy(iint->ima_hash, &hash, length);
> > - iint->version = i_version;
> > - iint->flags |= IMA_COLLECTED;
> > - } else
> > - result = -ENOMEM;
> > - }
> > + /* Initialize hash digest to 0's in case of failure */
> > + memset(&hash.digest, 0, sizeof(hash.digest));
> > +
> > + if (buf)
> > + result = ima_calc_buffer_hash(buf, size, &hash.hdr);
> > + else
> > + result = ima_calc_file_hash(file, &hash.hdr);
> > +
> > + if (result && result != -EBADF && result != -EINVAL)
> > + goto out;
> > +
> > + length = sizeof(hash.hdr) + hash.hdr.length;
> > + tmpbuf = krealloc(iint->ima_hash, length, GFP_NOFS);
> > + if (!tmpbuf) {
> > + result = -ENOMEM;
> > + goto out;
> > }
> > +
> > + iint->ima_hash = tmpbuf;
> > + memcpy(iint->ima_hash, &hash, length);
> > + iint->version = i_version;
> > +
> > + /* Possibly temporary failure due to type of read (eg. O_DIRECT) */
> > + if (result != -EBADF && result != -EINVAL)
> > + iint->flags |= IMA_COLLECTED;
>
>
> Result can be other than 0, EBADF and EINVAL here?
> It is confusing.. simpler than can be just
>
> if (!result)
> iint->flags |= IMA_COLLECTED;
>
> Isn't it?
Yes, that is better.
Mimi
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 10:09:21
|
Looks good to me.
On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> 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.
>
> (Based on Christoph's original patch.)
>
> Signed-off-by: Christoph Hellwig <hc...@ls...>
> Cc: Matthew Garrett <mj...@sr...>
> 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...>
>
> ---
> Changelog v6:
> - defined separate efivarfs and libfs patches.
>
> Changelog v5:
> - removed ocf2 and gfs2 integrity_read support.
> - removed ext4 special case to fail O_DIRECT open for buffered read.
>
> Changelog v4:
> - define ext2/4 specific ->integrity_read functions.
> - properly fail file open with O_DIRECT on filesystem not mounted
> with "-o dax".
>
> Changelog v3:
> - define simple_read_iter_from_buffer
> - replace the existing efivarfs ->read method with ->read_iter method.
> - squashed other fs definitions of ->integrity_read with this patch.
>
> Changelog v2:
> - change iovec to kvec
>
> Changelog v1:
> - update the patch description, removing the concept that the presence of
> ->integrity_read indicates that the file system can support IMA. (Mimi)
>
> fs/btrfs/file.c | 1 +
> fs/efivarfs/file.c | 1 +
> fs/ext2/file.c | 17 +++++++++++++++++
> fs/ext4/file.c | 20 ++++++++++++++++++++
> fs/f2fs/file.c | 1 +
> fs/jffs2/file.c | 1 +
> fs/jfs/file.c | 1 +
> fs/nilfs2/file.c | 1 +
> fs/ramfs/file-mmu.c | 1 +
> fs/ramfs/file-nommu.c | 1 +
> fs/ubifs/file.c | 1 +
> fs/xfs/xfs_file.c | 21 +++++++++++++++++++++
> include/linux/fs.h | 1 +
> mm/shmem.c | 1 +
> security/integrity/iint.c | 20 ++++++++++++++------
> 15 files changed, 83 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 9e75d8a39aac..2542dc66c85c 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -3125,6 +3125,7 @@ const struct file_operations btrfs_file_operations = {
> #endif
> .clone_file_range = btrfs_clone_file_range,
> .dedupe_file_range = btrfs_dedupe_file_range,
> + .integrity_read = generic_file_read_iter,
> };
>
> void btrfs_auto_defrag_exit(void)
> diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
> index 863f1b100165..17955a92a5b3 100644
> --- a/fs/efivarfs/file.c
> +++ b/fs/efivarfs/file.c
> @@ -179,4 +179,5 @@ const struct file_operations efivarfs_file_operations = {
> .write = efivarfs_file_write,
> .llseek = no_llseek,
> .unlocked_ioctl = efivarfs_file_ioctl,
> + .integrity_read = efivarfs_file_read_iter,
> };
> diff --git a/fs/ext2/file.c b/fs/ext2/file.c
> index d34d32bdc944..111069de1973 100644
> --- a/fs/ext2/file.c
> +++ b/fs/ext2/file.c
> @@ -192,6 +192,22 @@ static ssize_t ext2_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
> return generic_file_read_iter(iocb, to);
> }
>
> +static ssize_t ext2_file_integrity_read_iter(struct kiocb *iocb,
> + struct iov_iter *to)
> +{
> + struct inode *inode = file_inode(iocb->ki_filp);
> +
> + lockdep_assert_held(&inode->i_rwsem);
> +#ifdef CONFIG_FS_DAX
> + if (!iov_iter_count(to))
> + return 0; /* skip atime */
> +
> + if (IS_DAX(iocb->ki_filp->f_mapping->host))
> + return dax_iomap_rw(iocb, to, &ext2_iomap_ops);
> +#endif
> + return generic_file_read_iter(iocb, to);
> +}
> +
> static ssize_t ext2_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
> {
> #ifdef CONFIG_FS_DAX
> @@ -216,6 +232,7 @@ const struct file_operations ext2_file_operations = {
> .get_unmapped_area = thp_get_unmapped_area,
> .splice_read = generic_file_splice_read,
> .splice_write = iter_file_splice_write,
> + .integrity_read = ext2_file_integrity_read_iter,
> };
>
> const struct inode_operations ext2_file_inode_operations = {
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 58294c9a7e1d..3ab4105c8578 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -74,6 +74,25 @@ static ssize_t ext4_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
> return generic_file_read_iter(iocb, to);
> }
>
> +static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb,
> + struct iov_iter *to)
> +{
> + struct inode *inode = file_inode(iocb->ki_filp);
> +
> + 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
> + return generic_file_read_iter(iocb, to);
> +}
> +
> /*
> * Called when an inode is released. Note that this is different
> * from ext4_file_open: open gets called at every open, but release
> @@ -747,6 +766,7 @@ const struct file_operations ext4_file_operations = {
> .splice_read = generic_file_splice_read,
> .splice_write = iter_file_splice_write,
> .fallocate = ext4_fallocate,
> + .integrity_read = ext4_file_integrity_read_iter,
> };
>
> const struct inode_operations ext4_file_inode_operations = {
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 2706130c261b..82ea81da0b2d 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2514,4 +2514,5 @@ const struct file_operations f2fs_file_operations = {
> #endif
> .splice_read = generic_file_splice_read,
> .splice_write = iter_file_splice_write,
> + .integrity_read = generic_file_read_iter,
> };
> diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
> index c12476e309c6..5a63034cccf5 100644
> --- a/fs/jffs2/file.c
> +++ b/fs/jffs2/file.c
> @@ -57,6 +57,7 @@ const struct file_operations jffs2_file_operations =
> .mmap = generic_file_readonly_mmap,
> .fsync = jffs2_fsync,
> .splice_read = generic_file_splice_read,
> + .integrity_read = generic_file_read_iter,
> };
>
> /* jffs2_file_inode_operations */
> diff --git a/fs/jfs/file.c b/fs/jfs/file.c
> index 739492c7a3fd..423512a810e4 100644
> --- a/fs/jfs/file.c
> +++ b/fs/jfs/file.c
> @@ -162,4 +162,5 @@ const struct file_operations jfs_file_operations = {
> #ifdef CONFIG_COMPAT
> .compat_ioctl = jfs_compat_ioctl,
> #endif
> + .integrity_read = generic_file_read_iter,
> };
> diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
> index c5fa3dee72fc..55e058ac487f 100644
> --- a/fs/nilfs2/file.c
> +++ b/fs/nilfs2/file.c
> @@ -150,6 +150,7 @@ const struct file_operations nilfs_file_operations = {
> /* .release = nilfs_release_file, */
> .fsync = nilfs_sync_file,
> .splice_read = generic_file_splice_read,
> + .integrity_read = generic_file_read_iter,
> };
>
> const struct inode_operations nilfs_file_inode_operations = {
> diff --git a/fs/ramfs/file-mmu.c b/fs/ramfs/file-mmu.c
> index 12af0490322f..4f24d1b589b1 100644
> --- a/fs/ramfs/file-mmu.c
> +++ b/fs/ramfs/file-mmu.c
> @@ -47,6 +47,7 @@ const struct file_operations ramfs_file_operations = {
> .splice_write = iter_file_splice_write,
> .llseek = generic_file_llseek,
> .get_unmapped_area = ramfs_mmu_get_unmapped_area,
> + .integrity_read = generic_file_read_iter,
> };
>
> const struct inode_operations ramfs_file_inode_operations = {
> diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
> index 2ef7ce75c062..5ee704fa84e0 100644
> --- a/fs/ramfs/file-nommu.c
> +++ b/fs/ramfs/file-nommu.c
> @@ -50,6 +50,7 @@ const struct file_operations ramfs_file_operations = {
> .splice_read = generic_file_splice_read,
> .splice_write = iter_file_splice_write,
> .llseek = generic_file_llseek,
> + .integrity_read = generic_file_read_iter,
> };
>
> const struct inode_operations ramfs_file_inode_operations = {
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 8cad0b19b404..5e52a315e18b 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -1747,4 +1747,5 @@ const struct file_operations ubifs_file_operations = {
> #ifdef CONFIG_COMPAT
> .compat_ioctl = ubifs_compat_ioctl,
> #endif
> + .integrity_read = generic_file_read_iter,
> };
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index c4893e226fd8..0a6704b563d6 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -292,6 +292,26 @@ xfs_file_read_iter(
> return ret;
> }
>
> +static ssize_t
> +xfs_integrity_read(
> + struct kiocb *iocb,
> + struct iov_iter *to)
> +{
> + struct inode *inode = file_inode(iocb->ki_filp);
> + struct xfs_mount *mp = XFS_I(inode)->i_mount;
> +
> + lockdep_assert_held(&inode->i_rwsem);
> +
> + XFS_STATS_INC(mp, xs_read_calls);
> +
> + if (XFS_FORCED_SHUTDOWN(mp))
> + return -EIO;
> +
> + if (IS_DAX(inode))
> + return dax_iomap_rw(iocb, to, &xfs_iomap_ops);
> + return generic_file_read_iter(iocb, to);
> +}
> +
> /*
> * Zero any on disk space between the current EOF and the new, larger EOF.
> *
> @@ -1175,6 +1195,7 @@ const struct file_operations xfs_file_operations = {
> .fallocate = xfs_file_fallocate,
> .clone_file_range = xfs_file_clone_range,
> .dedupe_file_range = xfs_file_dedupe_range,
> + .integrity_read = xfs_integrity_read,
> };
>
> const struct file_operations xfs_dir_file_operations = {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index fdec9b763b54..8d0d10e1dd93 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1699,6 +1699,7 @@ struct file_operations {
> u64);
> ssize_t (*dedupe_file_range)(struct file *, u64, u64, struct file *,
> u64);
> + ssize_t (*integrity_read)(struct kiocb *, struct iov_iter *);
> } __randomize_layout;
>
> struct inode_operations {
> diff --git a/mm/shmem.c b/mm/shmem.c
> index b0aa6075d164..805d99011ca4 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3849,6 +3849,7 @@ static const struct file_operations shmem_file_operations = {
> .splice_read = generic_file_splice_read,
> .splice_write = iter_file_splice_write,
> .fallocate = shmem_fallocate,
> + .integrity_read = shmem_file_read_iter,
> #endif
> };
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 6fc888ca468e..df04f35a1d40 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -21,6 +21,7 @@
> #include <linux/rbtree.h>
> #include <linux/file.h>
> #include <linux/uaccess.h>
> +#include <linux/uio.h>
> #include "integrity.h"
>
> static struct rb_root integrity_iint_tree = RB_ROOT;
> @@ -184,18 +185,25 @@ security_initcall(integrity_iintcache_init);
> int integrity_kernel_read(struct file *file, loff_t offset,
> void *addr, unsigned long count)
> {
> - mm_segment_t old_fs;
> - char __user *buf = (char __user *)addr;
> + struct inode *inode = file_inode(file);
> + struct kvec iov = { .iov_base = addr, .iov_len = count };
> + struct kiocb kiocb;
> + struct iov_iter iter;
> ssize_t ret;
>
> + lockdep_assert_held(&inode->i_rwsem);
> +
> if (!(file->f_mode & FMODE_READ))
> return -EBADF;
> + if (!file->f_op->integrity_read)
> + return -EBADF;
>
> - old_fs = get_fs();
> - set_fs(get_ds());
> - ret = __vfs_read(file, buf, count, &offset);
> - set_fs(old_fs);
> + init_sync_kiocb(&kiocb, file);
> + kiocb.ki_pos = offset;
> + iov_iter_kvec(&iter, READ | ITER_KVEC, &iov, 1, count);
>
> + ret = file->f_op->integrity_read(&kiocb, &iter);
> + BUG_ON(ret == -EIOCBQUEUED);
> return ret;
> }
>
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to maj...@vg...
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Dmitry
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 10:07:58
|
Looks good to me.
On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
> Permit normally denied access/execute permission for files in policy
> on IMA unsupported filesystems. This patch defines "fs_unsafe", a
> builtin policy.
>
> Signed-off-by: Mimi Zohar <zo...@li...>
>
> ---
> Changelog v3:
> - include dont_failsafe rule when displaying policy
>
> Documentation/admin-guide/kernel-parameters.txt | 8 +++++++-
> security/integrity/ima/ima_policy.c | 12 ++++++++++++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index d9c171ce4190..4e303be83df6 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1502,7 +1502,7 @@
>
> ima_policy= [IMA]
> The builtin policies to load during IMA setup.
> - Format: "tcb | appraise_tcb | secure_boot"
> + Format: "tcb | appraise_tcb | secure_boot | fs_unsafe"
>
> The "tcb" policy measures all programs exec'd, files
> mmap'd for exec, and all files opened with the read
> @@ -1517,6 +1517,12 @@
> of files (eg. kexec kernel image, kernel modules,
> firmware, policy, etc) based on file signatures.
>
> + The "fs_unsafe" policy permits normally denied
> + access/execute permission for files in policy on IMA
> + unsupported filesystems. Note this option, as the
> + name implies, is not safe and not recommended for
> + any environments other than testing.
> +
> ima_tcb [IMA] Deprecated. Use ima_policy= instead.
> Load a policy which meets the needs of the Trusted
> Computing Base. This means IMA will measure all
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 43b85a4fb8e8..cddd9dfb01e1 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -169,6 +169,10 @@ static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
> .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
> };
>
> +static struct ima_rule_entry dont_failsafe_rules[] __ro_after_init = {
> + {.action = DONT_FAILSAFE}
> +};
> +
> static LIST_HEAD(ima_default_rules);
> static LIST_HEAD(ima_policy_rules);
> static LIST_HEAD(ima_temp_rules);
> @@ -188,6 +192,7 @@ __setup("ima_tcb", default_measure_policy_setup);
>
> static bool ima_use_appraise_tcb __initdata;
> static bool ima_use_secure_boot __initdata;
> +static bool ima_use_dont_failsafe __initdata;
> static int __init policy_setup(char *str)
> {
> char *p;
> @@ -201,6 +206,10 @@ static int __init policy_setup(char *str)
> ima_use_appraise_tcb = 1;
> else if (strcmp(p, "secure_boot") == 0)
> ima_use_secure_boot = 1;
> + else if (strcmp(p, "fs_unsafe") == 0) {
> + ima_use_dont_failsafe = 1;
> + set_failsafe(0);
> + }
> }
>
> return 1;
> @@ -470,6 +479,9 @@ void __init ima_init_policy(void)
> temp_ima_appraise |= IMA_APPRAISE_POLICY;
> }
>
> + if (ima_use_dont_failsafe)
> + list_add_tail(&dont_failsafe_rules[0].list, &ima_default_rules);
> +
> ima_rules = &ima_default_rules;
> ima_update_policy_flag();
> }
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to maj...@vg...
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Dmitry
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 10:07:28
|
On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
> Permit normally denied access/execute permission for files in policy
> on IMA unsupported filesystems. This patch defines the "dont_failsafe"
> policy action rule.
>
> Signed-off-by: Mimi Zohar <zo...@li...>
>
> ---
> Changelog v3:
> - include dont_failsafe rule when displaying policy
> - fail attempt to add dont_failsafe rule when appending to the policy
>
> Documentation/ABI/testing/ima_policy | 3 ++-
> security/integrity/ima/ima.h | 1 +
> security/integrity/ima/ima_main.c | 12 +++++++++++-
> security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++++++-
> 4 files changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index e76432b9954d..f271207743e5 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -17,7 +17,8 @@ Description:
>
> rule format: action [condition ...]
>
> - action: measure | dont_measure | appraise | dont_appraise | audit
> + action: measure | dont_meaure | appraise | dont_appraise |
> + audit | dont_failsafe
> condition:= base | lsm [option]
> base: [[func=] [mask=] [fsmagic=] [fsuuid=] [uid=]
> [euid=] [fowner=]]
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index d52b487ad259..c5f34f7c5b0f 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -224,6 +224,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos);
> void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> void ima_policy_stop(struct seq_file *m, void *v);
> int ima_policy_show(struct seq_file *m, void *v);
> +void set_failsafe(bool flag);
>
> /* Appraise integrity measurements */
> #define IMA_APPRAISE_ENFORCE 0x01
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index d23dfe6ede18..b00186914df8 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -38,6 +38,12 @@ int ima_appraise;
> int ima_hash_algo = HASH_ALGO_SHA1;
> static int hash_setup_done;
>
> +static bool ima_failsafe = 1;
> +void set_failsafe(bool flag)
> +{
> + ima_failsafe = flag;
> +}
> +
> static int __init hash_setup(char *str)
> {
> struct ima_template_desc *template_desc = ima_template_desc_current();
> @@ -260,8 +266,12 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> __putname(pathbuf);
> out:
> inode_unlock(inode);
> - if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
> + if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> + if (!ima_failsafe && rc == -EBADF)
> + return 0;
> +
By default IMA is failsafe. ima_failsafe is true.
Return 0 is needed in failsafe mode. right?
But in this logic it will happen if ima_failsafe is false. meaning it
is not failsafe.
Is it a typo?
> return -EACCES;
> + }
> return 0;
> }
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 95209a5f8595..43b85a4fb8e8 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -40,12 +40,14 @@
> #define APPRAISE 0x0004 /* same as IMA_APPRAISE */
> #define DONT_APPRAISE 0x0008
> #define AUDIT 0x0040
> +#define DONT_FAILSAFE 0x0400
>
> #define INVALID_PCR(a) (((a) < 0) || \
> (a) >= (FIELD_SIZEOF(struct integrity_iint_cache, measured_pcrs) * 8))
>
> int ima_policy_flag;
> static int temp_ima_appraise;
> +static bool temp_failsafe = 1;
>
> #define MAX_LSM_RULES 6
> enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
> @@ -513,6 +515,9 @@ void ima_update_policy(void)
> if (ima_rules != policy) {
> ima_policy_flag = 0;
> ima_rules = policy;
> +
> + /* Only update on initial policy replacement, not append */
> + set_failsafe(temp_failsafe);
> }
> ima_update_policy_flag();
> }
> @@ -529,7 +534,7 @@ enum {
> Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
> Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
> Opt_appraise_type, Opt_permit_directio,
> - Opt_pcr
> + Opt_pcr, Opt_dont_failsafe
> };
>
> static match_table_t policy_tokens = {
> @@ -560,6 +565,7 @@ static match_table_t policy_tokens = {
> {Opt_appraise_type, "appraise_type=%s"},
> {Opt_permit_directio, "permit_directio"},
> {Opt_pcr, "pcr=%s"},
> + {Opt_dont_failsafe, "dont_failsafe"},
> {Opt_err, NULL}
> };
>
> @@ -630,6 +636,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> if ((*p == '\0') || (*p == ' ') || (*p == '\t'))
> continue;
> token = match_token(p, policy_tokens, args);
> + if (entry->action == DONT_FAILSAFE) {
> + /* no args permitted, force invalid rule */
> + token = Opt_dont_failsafe;
> + }
> +
> switch (token) {
> case Opt_measure:
> ima_log_string(ab, "action", "measure");
> @@ -671,6 +682,19 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>
> entry->action = AUDIT;
> break;
> + case Opt_dont_failsafe:
> + ima_log_string(ab, "action", "dont_failsafe");
> +
> + if (entry->action != UNKNOWN)
> + result = -EINVAL;
> +
> + /* Permit on initial policy replacement only */
> + if (ima_rules != &ima_policy_rules)
> + temp_failsafe = 0;
> + else
> + result = -EINVAL;
> + entry->action = DONT_FAILSAFE;
> + break;
> case Opt_func:
> ima_log_string(ab, "func", args[0].from);
>
> @@ -949,6 +973,7 @@ void ima_delete_rules(void)
> int i;
>
> temp_ima_appraise = 0;
> + temp_failsafe = 1;
> list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) {
> for (i = 0; i < MAX_LSM_RULES; i++)
> kfree(entry->lsm[i].args_p);
> @@ -1040,6 +1065,8 @@ int ima_policy_show(struct seq_file *m, void *v)
> seq_puts(m, pt(Opt_dont_appraise));
> if (entry->action & AUDIT)
> seq_puts(m, pt(Opt_audit));
> + if (entry->action & DONT_FAILSAFE)
> + seq_puts(m, pt(Opt_dont_failsafe));
>
> seq_puts(m, " ");
>
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to maj...@vg...
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Dmitry
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 10:06:06
|
On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
> All files matching a "measure" rule must be included in the IMA
> measurement list, even when the file hash cannot be calculated.
> Similarly, all files matching an "audit" rule must be audited, even when
> the file hash can not be calculated.
>
> The file data hash field contained in the IMA measurement list template
> data will contain 0's instead of the actual file hash digest.
>
> Signed-off-by: Mimi Zohar <zo...@li...>
>
> ---
> Changelog v6:
> - replace "?:" with if/then
> - annotate i_version usage
> - reword O_DIRECT comment
>
> Changelog v5:
> - Fail files opened O_DIRECT, but include attempt in measurement list.
>
> Changelog v4:
> - Based on both -EBADF and -EINVAL
> - clean up ima_collect_measurement()
>
> security/integrity/ima/ima_api.c | 67 +++++++++++++++++++++++--------------
> security/integrity/ima/ima_crypto.c | 10 ++++++
> security/integrity/ima/ima_main.c | 7 ++--
> 3 files changed, 54 insertions(+), 30 deletions(-)
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index c2edba8de35e..1dee695642a4 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -199,42 +199,59 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> struct inode *inode = file_inode(file);
> const char *filename = file->f_path.dentry->d_name.name;
> int result = 0;
> + int length;
> + void *tmpbuf;
> + u64 i_version;
> struct {
> struct ima_digest_data hdr;
> char digest[IMA_MAX_DIGEST_SIZE];
> } hash;
>
> - if (!(iint->flags & IMA_COLLECTED)) {
> - u64 i_version = file_inode(file)->i_version;
> + if (iint->flags & IMA_COLLECTED)
> + goto out;
>
> - if (file->f_flags & O_DIRECT) {
> - audit_cause = "failed(directio)";
> - result = -EACCES;
> - goto out;
> - }
> + /*
> + * Dectecting file change is based on i_version. On filesystems
> + * which do not support i_version, support is limited to an initial
> + * measurement/appraisal/audit.
> + */
> + i_version = file_inode(file)->i_version;
> + hash.hdr.algo = algo;
>
> - hash.hdr.algo = algo;
> -
> - result = (!buf) ? ima_calc_file_hash(file, &hash.hdr) :
> - ima_calc_buffer_hash(buf, size, &hash.hdr);
> - if (!result) {
> - int length = sizeof(hash.hdr) + hash.hdr.length;
> - void *tmpbuf = krealloc(iint->ima_hash, length,
> - GFP_NOFS);
> - if (tmpbuf) {
> - iint->ima_hash = tmpbuf;
> - memcpy(iint->ima_hash, &hash, length);
> - iint->version = i_version;
> - iint->flags |= IMA_COLLECTED;
> - } else
> - result = -ENOMEM;
> - }
> + /* Initialize hash digest to 0's in case of failure */
> + memset(&hash.digest, 0, sizeof(hash.digest));
> +
> + if (buf)
> + result = ima_calc_buffer_hash(buf, size, &hash.hdr);
> + else
> + result = ima_calc_file_hash(file, &hash.hdr);
> +
> + if (result && result != -EBADF && result != -EINVAL)
> + goto out;
> +
> + length = sizeof(hash.hdr) + hash.hdr.length;
> + tmpbuf = krealloc(iint->ima_hash, length, GFP_NOFS);
> + if (!tmpbuf) {
> + result = -ENOMEM;
> + goto out;
> }
> +
> + iint->ima_hash = tmpbuf;
> + memcpy(iint->ima_hash, &hash, length);
> + iint->version = i_version;
> +
> + /* Possibly temporary failure due to type of read (eg. O_DIRECT) */
> + if (result != -EBADF && result != -EINVAL)
> + iint->flags |= IMA_COLLECTED;
Result can be other than 0, EBADF and EINVAL here?
It is confusing.. simpler than can be just
if (!result)
iint->flags |= IMA_COLLECTED;
Isn't it?
> out:
> - if (result)
> + if (result) {
> + if (file->f_flags & O_DIRECT)
> + audit_cause = "failed(directio)";
> +
> integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
> filename, "collect_data", audit_cause,
> result, 0);
> + }
> return result;
> }
>
> @@ -278,7 +295,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
> }
>
> result = ima_store_template(entry, violation, inode, filename, pcr);
> - if (!result || result == -EEXIST) {
> + if ((!result || result == -EEXIST) && !(file->f_flags & O_DIRECT)) {
> iint->flags |= IMA_MEASURED;
> iint->measured_pcrs |= (0x1 << pcr);
> }
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 802d5d20f36f..a856d8c9c9f3 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -441,6 +441,16 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> loff_t i_size;
> int rc;
>
> + /*
> + * For consistency, fail file's opened with the O_DIRECT flag on
> + * filesystems mounted with/without DAX option.
> + */
> + if (file->f_flags & O_DIRECT) {
> + hash->length = hash_digest_size[ima_hash_algo];
> + hash->algo = ima_hash_algo;
> + return -EINVAL;
> + }
> +
> i_size = i_size_read(file_inode(file));
>
> if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 2aebb7984437..d23dfe6ede18 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -235,11 +235,8 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
>
> rc = ima_collect_measurement(iint, file, buf, size, hash_algo);
> - if (rc != 0) {
> - if (file->f_flags & O_DIRECT)
> - rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES;
> + if (rc != 0 && rc != -EBADF && rc != -EINVAL)
> goto out_digsig;
> - }
>
> if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */
> pathname = ima_d_path(&file->f_path, &pathbuf, filename);
> @@ -247,7 +244,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> if (action & IMA_MEASURE)
> ima_store_measurement(iint, file, pathname,
> xattr_value, xattr_len, pcr);
> - if (action & IMA_APPRAISE_SUBMASK)
> + if (rc == 0 && (action & IMA_APPRAISE_SUBMASK))
> rc = ima_appraise_measurement(func, iint, file, pathname,
> xattr_value, xattr_len, opened);
> if (action & IMA_AUDIT)
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to maj...@vg...
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Dmitry
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 10:04:45
|
Looks good to me.
On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
> In preparation for defining an integrity_read file operation
> method for efivarfs, define a simple_read_iter_from_buffer()
> function.
>
> (Based on Al's code as posted in thread.)
>
> Suggested-by: Al Viro <vi...@ze...>
> Signed-off-by: Mimi Zohar <zo...@li...>
> Cc: Matthew Garrett <mj...@sr...>
>
> ---
> Changelog v6:
> - defined as a separate patch
>
> fs/libfs.c | 32 ++++++++++++++++++++++++++++++++
> include/linux/fs.h | 2 ++
> 2 files changed, 34 insertions(+)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 3aabe553fc45..b6e304c6828b 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -16,6 +16,7 @@
> #include <linux/exportfs.h>
> #include <linux/writeback.h>
> #include <linux/buffer_head.h> /* sync_mapping_buffers */
> +#include <linux/uio.h>
>
> #include <linux/uaccess.h>
>
> @@ -676,6 +677,37 @@ ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
> EXPORT_SYMBOL(simple_write_to_buffer);
>
> /**
> + * simple_read_iter_from_buffer - copy data from the buffer to user space
> + * @iocb: struct containing the file, the current position and other info
> + * @to: the user space buffer to read to
> + * @from: the buffer to read from
> + * @available: the size of the buffer
> + *
> + * The simple_read_iter_from_buffer() function reads up to @available bytes
> + * from the current buffer into the user space buffer.
> + *
> + * On success, the current buffer offset is advanced by the number of bytes
> + * read, or a negative value is returned on error.
> + **/
> +ssize_t simple_read_iter_from_buffer(struct kiocb *iocb, struct iov_iter *to,
> + const void *from, size_t available)
> +{
> + loff_t pos = iocb->ki_pos;
> + size_t ret;
> +
> + if (pos < 0)
> + return -EINVAL;
> + if (pos >= available)
> + return 0;
> + ret = copy_to_iter(from + pos, available - pos, to);
> + if (!ret && iov_iter_count(to))
> + return -EFAULT;
> + iocb->ki_pos = pos + ret;
> + return ret;
> +}
> +EXPORT_SYMBOL(simple_read_iter_from_buffer);
> +
> +/**
> * memory_read_from_buffer - copy data from the buffer
> * @to: the kernel space buffer to read to
> * @count: the maximum number of bytes to read
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 6e1fd5d21248..fdec9b763b54 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3097,6 +3097,8 @@ extern void simple_release_fs(struct vfsmount **mount, int *count);
>
> extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
> loff_t *ppos, const void *from, size_t available);
> +extern ssize_t simple_read_iter_from_buffer(struct kiocb *iocb,
> + struct iov_iter *to, const void *from, size_t available);
> extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
> const void __user *from, size_t count);
>
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to maj...@vg...
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Dmitry
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 09:59:32
|
Seems to look good from IMA side.
On Wed, Jul 26, 2017 at 4:22 PM, Mimi Zohar <zo...@li...> 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...>
>
> Changelog v4:
> - define ext2/4 specific ->integrity_read functions.
> - properly fail file open with O_DIRECT on filesystem not mounted
> with "-o dax".
>
> ---
> Changelog v3:
> - define simple_read_iter_from_buffer
> - replace the existing efivarfs ->read method with ->read_iter method.
> - squashed other fs definitions of ->integrity_read with this patch.
>
> Changelog v2:
> - change iovec to kvec
>
> Changelog v1:
> - update the patch description, removing the concept that the presence of
> ->integrity_read indicates that the file system can support IMA. (Mimi)
>
> fs/btrfs/file.c | 1 +
> fs/efivarfs/file.c | 12 +++++++-----
> fs/ext2/file.c | 17 +++++++++++++++++
> fs/ext4/file.c | 23 +++++++++++++++++++++++
> fs/f2fs/file.c | 1 +
> fs/gfs2/file.c | 2 ++
> fs/jffs2/file.c | 1 +
> fs/jfs/file.c | 1 +
> fs/libfs.c | 32 ++++++++++++++++++++++++++++++++
> fs/nilfs2/file.c | 1 +
> fs/ocfs2/file.c | 1 +
> fs/ramfs/file-mmu.c | 1 +
> fs/ramfs/file-nommu.c | 1 +
> fs/ubifs/file.c | 1 +
> fs/xfs/xfs_file.c | 21 +++++++++++++++++++++
> include/linux/fs.h | 3 +++
> mm/shmem.c | 1 +
> security/integrity/iint.c | 20 ++++++++++++++------
> 18 files changed, 129 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 9e75d8a39aac..2542dc66c85c 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -3125,6 +3125,7 @@ const struct file_operations btrfs_file_operations = {
> #endif
> .clone_file_range = btrfs_clone_file_range,
> .dedupe_file_range = btrfs_dedupe_file_range,
> + .integrity_read = generic_file_read_iter,
> };
>
> void btrfs_auto_defrag_exit(void)
> diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
> index 5f22e74bbade..17955a92a5b3 100644
> --- a/fs/efivarfs/file.c
> +++ b/fs/efivarfs/file.c
> @@ -64,9 +64,10 @@ static ssize_t efivarfs_file_write(struct file *file,
> return bytes;
> }
>
> -static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
> - size_t count, loff_t *ppos)
> +static ssize_t efivarfs_file_read_iter(struct kiocb *iocb,
> + struct iov_iter *iter)
> {
> + struct file *file = iocb->ki_filp;
> struct efivar_entry *var = file->private_data;
> unsigned long datasize = 0;
> u32 attributes;
> @@ -96,8 +97,8 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
> goto out_free;
>
> memcpy(data, &attributes, sizeof(attributes));
> - size = simple_read_from_buffer(userbuf, count, ppos,
> - data, datasize + sizeof(attributes));
> + size = simple_read_iter_from_buffer(iocb, iter, data,
> + datasize + sizeof(attributes));
> out_free:
> kfree(data);
>
> @@ -174,8 +175,9 @@ efivarfs_file_ioctl(struct file *file, unsigned int cmd, unsigned long p)
>
> const struct file_operations efivarfs_file_operations = {
> .open = simple_open,
> - .read = efivarfs_file_read,
> + .read_iter = efivarfs_file_read_iter,
> .write = efivarfs_file_write,
> .llseek = no_llseek,
> .unlocked_ioctl = efivarfs_file_ioctl,
> + .integrity_read = efivarfs_file_read_iter,
> };
> diff --git a/fs/ext2/file.c b/fs/ext2/file.c
> index d34d32bdc944..111069de1973 100644
> --- a/fs/ext2/file.c
> +++ b/fs/ext2/file.c
> @@ -192,6 +192,22 @@ static ssize_t ext2_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
> return generic_file_read_iter(iocb, to);
> }
>
> +static ssize_t ext2_file_integrity_read_iter(struct kiocb *iocb,
> + struct iov_iter *to)
> +{
> + struct inode *inode = file_inode(iocb->ki_filp);
> +
> + lockdep_assert_held(&inode->i_rwsem);
> +#ifdef CONFIG_FS_DAX
> + if (!iov_iter_count(to))
> + return 0; /* skip atime */
> +
> + if (IS_DAX(iocb->ki_filp->f_mapping->host))
> + return dax_iomap_rw(iocb, to, &ext2_iomap_ops);
> +#endif
> + return generic_file_read_iter(iocb, to);
> +}
> +
> static ssize_t ext2_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
> {
> #ifdef CONFIG_FS_DAX
> @@ -216,6 +232,7 @@ const struct file_operations ext2_file_operations = {
> .get_unmapped_area = thp_get_unmapped_area,
> .splice_read = generic_file_splice_read,
> .splice_write = iter_file_splice_write,
> + .integrity_read = ext2_file_integrity_read_iter,
> };
>
> const struct inode_operations ext2_file_inode_operations = {
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 58294c9a7e1d..cb423fff935f 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -74,6 +74,28 @@ static ssize_t ext4_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
> return generic_file_read_iter(iocb, to);
> }
>
> +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);
> +}
> +
> /*
> * Called when an inode is released. Note that this is different
> * from ext4_file_open: open gets called at every open, but release
> @@ -747,6 +769,7 @@ const struct file_operations ext4_file_operations = {
> .splice_read = generic_file_splice_read,
> .splice_write = iter_file_splice_write,
> .fallocate = ext4_fallocate,
> + .integrity_read = ext4_file_integrity_read_iter,
> };
>
> const struct inode_operations ext4_file_inode_operations = {
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 2706130c261b..82ea81da0b2d 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2514,4 +2514,5 @@ const struct file_operations f2fs_file_operations = {
> #endif
> .splice_read = generic_file_splice_read,
> .splice_write = iter_file_splice_write,
> + .integrity_read = generic_file_read_iter,
> };
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index c2062a108d19..9b49d09ba180 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -1124,6 +1124,7 @@ const struct file_operations gfs2_file_fops = {
> .splice_write = gfs2_file_splice_write,
> .setlease = simple_nosetlease,
> .fallocate = gfs2_fallocate,
> + .integrity_read = generic_file_read_iter,
> };
>
> const struct file_operations gfs2_dir_fops = {
> @@ -1152,6 +1153,7 @@ const struct file_operations gfs2_file_fops_nolock = {
> .splice_write = gfs2_file_splice_write,
> .setlease = generic_setlease,
> .fallocate = gfs2_fallocate,
> + .integrity_read = generic_file_read_iter,
> };
>
> const struct file_operations gfs2_dir_fops_nolock = {
> diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
> index c12476e309c6..5a63034cccf5 100644
> --- a/fs/jffs2/file.c
> +++ b/fs/jffs2/file.c
> @@ -57,6 +57,7 @@ const struct file_operations jffs2_file_operations =
> .mmap = generic_file_readonly_mmap,
> .fsync = jffs2_fsync,
> .splice_read = generic_file_splice_read,
> + .integrity_read = generic_file_read_iter,
> };
>
> /* jffs2_file_inode_operations */
> diff --git a/fs/jfs/file.c b/fs/jfs/file.c
> index 739492c7a3fd..423512a810e4 100644
> --- a/fs/jfs/file.c
> +++ b/fs/jfs/file.c
> @@ -162,4 +162,5 @@ const struct file_operations jfs_file_operations = {
> #ifdef CONFIG_COMPAT
> .compat_ioctl = jfs_compat_ioctl,
> #endif
> + .integrity_read = generic_file_read_iter,
> };
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 3aabe553fc45..99333264a0a7 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -16,6 +16,7 @@
> #include <linux/exportfs.h>
> #include <linux/writeback.h>
> #include <linux/buffer_head.h> /* sync_mapping_buffers */
> +#include <linux/uio.h>
>
> #include <linux/uaccess.h>
>
> @@ -676,6 +677,37 @@ ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
> EXPORT_SYMBOL(simple_write_to_buffer);
>
> /**
> + * simple_read_iter_from_buffer - copy data from the buffer to user space
> + * @iocb: struct containing the file, the current position and other info
> + * @to: the user space buffer to read to
> + * @from: the buffer to read from
> + * @available: the size of the buffer
> + *
> + * The simple_read_iter_from_buffer() function reads up to @available bytes
> + * from the current buffer into the user space buffer.
> + *
> + * On success, the current buffer offset is advanced by the number of bytes
> + * read, or a negative value is returned on error.
> + **/
> +ssize_t simple_read_iter_from_buffer(struct kiocb *iocb, struct iov_iter *to,
> + const void *from, size_t available)
> +{
> + loff_t pos = iocb->ki_pos;
> + size_t ret;
> +
> + if (pos < 0)
> + return -EINVAL;
> + if (pos >= available)
> + return 0;
> + ret = copy_to_iter(from + pos, available - pos, to);
> + if (!ret && iov_iter_count(to))
> + return -EFAULT;
> + iocb->ki_pos = pos + ret;
> + return ret;
> +}
> +EXPORT_SYMBOL(simple_read_iter_from_buffer);
> +
> +/**
> * memory_read_from_buffer - copy data from the buffer
> * @to: the kernel space buffer to read to
> * @count: the maximum number of bytes to read
> diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
> index c5fa3dee72fc..55e058ac487f 100644
> --- a/fs/nilfs2/file.c
> +++ b/fs/nilfs2/file.c
> @@ -150,6 +150,7 @@ const struct file_operations nilfs_file_operations = {
> /* .release = nilfs_release_file, */
> .fsync = nilfs_sync_file,
> .splice_read = generic_file_splice_read,
> + .integrity_read = generic_file_read_iter,
> };
>
> const struct inode_operations nilfs_file_inode_operations = {
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index bfeb647459d9..2832a7c92acd 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -2536,6 +2536,7 @@ const struct file_operations ocfs2_fops = {
> .fallocate = ocfs2_fallocate,
> .clone_file_range = ocfs2_file_clone_range,
> .dedupe_file_range = ocfs2_file_dedupe_range,
> + .integrity_read = ocfs2_file_read_iter,
> };
>
> const struct file_operations ocfs2_dops = {
> diff --git a/fs/ramfs/file-mmu.c b/fs/ramfs/file-mmu.c
> index 12af0490322f..4f24d1b589b1 100644
> --- a/fs/ramfs/file-mmu.c
> +++ b/fs/ramfs/file-mmu.c
> @@ -47,6 +47,7 @@ const struct file_operations ramfs_file_operations = {
> .splice_write = iter_file_splice_write,
> .llseek = generic_file_llseek,
> .get_unmapped_area = ramfs_mmu_get_unmapped_area,
> + .integrity_read = generic_file_read_iter,
> };
>
> const struct inode_operations ramfs_file_inode_operations = {
> diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
> index 2ef7ce75c062..5ee704fa84e0 100644
> --- a/fs/ramfs/file-nommu.c
> +++ b/fs/ramfs/file-nommu.c
> @@ -50,6 +50,7 @@ const struct file_operations ramfs_file_operations = {
> .splice_read = generic_file_splice_read,
> .splice_write = iter_file_splice_write,
> .llseek = generic_file_llseek,
> + .integrity_read = generic_file_read_iter,
> };
>
> const struct inode_operations ramfs_file_inode_operations = {
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 8cad0b19b404..5e52a315e18b 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -1747,4 +1747,5 @@ const struct file_operations ubifs_file_operations = {
> #ifdef CONFIG_COMPAT
> .compat_ioctl = ubifs_compat_ioctl,
> #endif
> + .integrity_read = generic_file_read_iter,
> };
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index c4893e226fd8..0a6704b563d6 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -292,6 +292,26 @@ xfs_file_read_iter(
> return ret;
> }
>
> +static ssize_t
> +xfs_integrity_read(
> + struct kiocb *iocb,
> + struct iov_iter *to)
> +{
> + struct inode *inode = file_inode(iocb->ki_filp);
> + struct xfs_mount *mp = XFS_I(inode)->i_mount;
> +
> + lockdep_assert_held(&inode->i_rwsem);
> +
> + XFS_STATS_INC(mp, xs_read_calls);
> +
> + if (XFS_FORCED_SHUTDOWN(mp))
> + return -EIO;
> +
> + if (IS_DAX(inode))
> + return dax_iomap_rw(iocb, to, &xfs_iomap_ops);
> + return generic_file_read_iter(iocb, to);
> +}
> +
> /*
> * Zero any on disk space between the current EOF and the new, larger EOF.
> *
> @@ -1175,6 +1195,7 @@ const struct file_operations xfs_file_operations = {
> .fallocate = xfs_file_fallocate,
> .clone_file_range = xfs_file_clone_range,
> .dedupe_file_range = xfs_file_dedupe_range,
> + .integrity_read = xfs_integrity_read,
> };
>
> const struct file_operations xfs_dir_file_operations = {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 6e1fd5d21248..8d0d10e1dd93 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1699,6 +1699,7 @@ struct file_operations {
> u64);
> ssize_t (*dedupe_file_range)(struct file *, u64, u64, struct file *,
> u64);
> + ssize_t (*integrity_read)(struct kiocb *, struct iov_iter *);
> } __randomize_layout;
>
> struct inode_operations {
> @@ -3097,6 +3098,8 @@ extern void simple_release_fs(struct vfsmount **mount, int *count);
>
> extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
> loff_t *ppos, const void *from, size_t available);
> +extern ssize_t simple_read_iter_from_buffer(struct kiocb *iocb,
> + struct iov_iter *to, const void *from, size_t available);
> extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
> const void __user *from, size_t count);
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index b0aa6075d164..805d99011ca4 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3849,6 +3849,7 @@ static const struct file_operations shmem_file_operations = {
> .splice_read = generic_file_splice_read,
> .splice_write = iter_file_splice_write,
> .fallocate = shmem_fallocate,
> + .integrity_read = shmem_file_read_iter,
> #endif
> };
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 6fc888ca468e..df04f35a1d40 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -21,6 +21,7 @@
> #include <linux/rbtree.h>
> #include <linux/file.h>
> #include <linux/uaccess.h>
> +#include <linux/uio.h>
> #include "integrity.h"
>
> static struct rb_root integrity_iint_tree = RB_ROOT;
> @@ -184,18 +185,25 @@ security_initcall(integrity_iintcache_init);
> int integrity_kernel_read(struct file *file, loff_t offset,
> void *addr, unsigned long count)
> {
> - mm_segment_t old_fs;
> - char __user *buf = (char __user *)addr;
> + struct inode *inode = file_inode(file);
> + struct kvec iov = { .iov_base = addr, .iov_len = count };
> + struct kiocb kiocb;
> + struct iov_iter iter;
> ssize_t ret;
>
> + lockdep_assert_held(&inode->i_rwsem);
> +
> if (!(file->f_mode & FMODE_READ))
> return -EBADF;
> + if (!file->f_op->integrity_read)
> + return -EBADF;
>
> - old_fs = get_fs();
> - set_fs(get_ds());
> - ret = __vfs_read(file, buf, count, &offset);
> - set_fs(old_fs);
> + init_sync_kiocb(&kiocb, file);
> + kiocb.ki_pos = offset;
> + iov_iter_kvec(&iter, READ | ITER_KVEC, &iov, 1, count);
>
> + ret = file->f_op->integrity_read(&kiocb, &iter);
> + BUG_ON(ret == -EIOCBQUEUED);
> return ret;
> }
>
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to maj...@vg...
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Dmitry
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 09:39:44
|
Also where is checking for DONT_FAILSAFE (enforcement)?
On Tue, Aug 22, 2017 at 12:34 PM, Dmitry Kasatkin
<dmi...@gm...> wrote:
> On Wed, Jul 26, 2017 at 4:22 PM, Mimi Zohar <zo...@li...> wrote:
>> Permit normally denied access/execute permission for files in policy
>> on IMA unsupported filesystems. This patch defines the "dont_failsafe"
>> policy action rule.
>>
>> Mimi Zohar <zo...@li...>
>>
>> ---
>> Changelog v3:
>> - include dont_failsafe rule when displaying policy
>> - fail attempt to add dont_failsafe rule when appending to the policy
>>
>> Documentation/ABI/testing/ima_policy | 3 ++-
>> security/integrity/ima/ima.h | 1 +
>> security/integrity/ima/ima_main.c | 11 ++++++++++-
>> security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++++++-
>> 4 files changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>> index e76432b9954d..f271207743e5 100644
>> --- a/Documentation/ABI/testing/ima_policy
>> +++ b/Documentation/ABI/testing/ima_policy
>> @@ -17,7 +17,8 @@ Description:
>>
>> rule format: action [condition ...]
>>
>> - action: measure | dont_measure | appraise | dont_appraise | audit
>> + action: measure | dont_meaure | appraise | dont_appraise |
>> + audit | dont_failsafe
>> condition:= base | lsm [option]
>> base: [[func=] [mask=] [fsmagic=] [fsuuid=] [uid=]
>> [euid=] [fowner=]]
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index d52b487ad259..c5f34f7c5b0f 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -224,6 +224,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos);
>> void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
>> void ima_policy_stop(struct seq_file *m, void *v);
>> int ima_policy_show(struct seq_file *m, void *v);
>> +void set_failsafe(bool flag);
>>
>> /* Appraise integrity measurements */
>> #define IMA_APPRAISE_ENFORCE 0x01
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index 3941371402ff..664edab0f758 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -38,6 +38,11 @@ int ima_appraise;
>> int ima_hash_algo = HASH_ALGO_SHA1;
>> static int hash_setup_done;
>>
>> +static bool ima_failsafe = 1;
>> +void set_failsafe(bool flag) {
>> + ima_failsafe = flag;
>> +}
>> +
>> static int __init hash_setup(char *str)
>> {
>> struct ima_template_desc *template_desc = ima_template_desc_current();
>> @@ -263,8 +268,12 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>> __putname(pathbuf);
>> out:
>> inode_unlock(inode);
>> - if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
>> + if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
>> + if (!ima_failsafe && rc == -EBADF)
>> + return 0;
>> +
>
> By default IMA is failsaif. ima_failsafe is true.
> Return 0 is needed in failsafe mode. right?
> But in this logic it will happen if ima_failsafe is false. meaning it
> is not failsafe.
> Is it a typo?
>
>
>> return -EACCES;
>> + }
>> return 0;
>> }
>>
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index 95209a5f8595..43b85a4fb8e8 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -40,12 +40,14 @@
>> #define APPRAISE 0x0004 /* same as IMA_APPRAISE */
>> #define DONT_APPRAISE 0x0008
>> #define AUDIT 0x0040
>> +#define DONT_FAILSAFE 0x0400
>>
>> #define INVALID_PCR(a) (((a) < 0) || \
>> (a) >= (FIELD_SIZEOF(struct integrity_iint_cache, measured_pcrs) * 8))
>>
>> int ima_policy_flag;
>> static int temp_ima_appraise;
>> +static bool temp_failsafe = 1;
>>
>> #define MAX_LSM_RULES 6
>> enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
>> @@ -513,6 +515,9 @@ void ima_update_policy(void)
>> if (ima_rules != policy) {
>> ima_policy_flag = 0;
>> ima_rules = policy;
>> +
>> + /* Only update on initial policy replacement, not append */
>> + set_failsafe(temp_failsafe);
>> }
>> ima_update_policy_flag();
>> }
>> @@ -529,7 +534,7 @@ enum {
>> Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
>> Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
>> Opt_appraise_type, Opt_permit_directio,
>> - Opt_pcr
>> + Opt_pcr, Opt_dont_failsafe
>> };
>>
>> static match_table_t policy_tokens = {
>> @@ -560,6 +565,7 @@ static match_table_t policy_tokens = {
>> {Opt_appraise_type, "appraise_type=%s"},
>> {Opt_permit_directio, "permit_directio"},
>> {Opt_pcr, "pcr=%s"},
>> + {Opt_dont_failsafe, "dont_failsafe"},
>> {Opt_err, NULL}
>> };
>>
>> @@ -630,6 +636,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>> if ((*p == '\0') || (*p == ' ') || (*p == '\t'))
>> continue;
>> token = match_token(p, policy_tokens, args);
>> + if (entry->action == DONT_FAILSAFE) {
>> + /* no args permitted, force invalid rule */
>> + token = Opt_dont_failsafe;
>> + }
>> +
>> switch (token) {
>> case Opt_measure:
>> ima_log_string(ab, "action", "measure");
>> @@ -671,6 +682,19 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>>
>> entry->action = AUDIT;
>> break;
>> + case Opt_dont_failsafe:
>> + ima_log_string(ab, "action", "dont_failsafe");
>> +
>> + if (entry->action != UNKNOWN)
>> + result = -EINVAL;
>> +
>> + /* Permit on initial policy replacement only */
>> + if (ima_rules != &ima_policy_rules)
>> + temp_failsafe = 0;
>> + else
>> + result = -EINVAL;
>> + entry->action = DONT_FAILSAFE;
>> + break;
>> case Opt_func:
>> ima_log_string(ab, "func", args[0].from);
>>
>> @@ -949,6 +973,7 @@ void ima_delete_rules(void)
>> int i;
>>
>> temp_ima_appraise = 0;
>> + temp_failsafe = 1;
>> list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) {
>> for (i = 0; i < MAX_LSM_RULES; i++)
>> kfree(entry->lsm[i].args_p);
>> @@ -1040,6 +1065,8 @@ int ima_policy_show(struct seq_file *m, void *v)
>> seq_puts(m, pt(Opt_dont_appraise));
>> if (entry->action & AUDIT)
>> seq_puts(m, pt(Opt_audit));
>> + if (entry->action & DONT_FAILSAFE)
>> + seq_puts(m, pt(Opt_dont_failsafe));
>>
>> seq_puts(m, " ");
>>
>> --
>> 2.7.4
>>
>>
>> ------------------------------------------------------------------------------
>> Check out the vibrant tech community on one of the world's most
>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>> _______________________________________________
>> Linux-ima-devel mailing list
>> Lin...@li...
>> https://lists.sourceforge.net/lists/listinfo/linux-ima-devel
>
>
>
> --
> Thanks,
> Dmitry
--
Thanks,
Dmitry
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 09:37:01
|
Looks good to me.
On Wed, Jul 26, 2017 at 4:22 PM, Mimi Zohar <zo...@li...> wrote:
> Permit normally denied access/execute permission for files in policy
> on IMA unsupported filesystems. This patch defines "fs_unsafe", a
> builtin policy.
>
> Mimi Zohar <zo...@li...>
>
> ---
> Changelog v3:
> - include dont_failsafe rule when displaying policy
>
> Documentation/admin-guide/kernel-parameters.txt | 8 +++++++-
> security/integrity/ima/ima_policy.c | 12 ++++++++++++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index d9c171ce4190..4e303be83df6 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1502,7 +1502,7 @@
>
> ima_policy= [IMA]
> The builtin policies to load during IMA setup.
> - Format: "tcb | appraise_tcb | secure_boot"
> + Format: "tcb | appraise_tcb | secure_boot | fs_unsafe"
>
> The "tcb" policy measures all programs exec'd, files
> mmap'd for exec, and all files opened with the read
> @@ -1517,6 +1517,12 @@
> of files (eg. kexec kernel image, kernel modules,
> firmware, policy, etc) based on file signatures.
>
> + The "fs_unsafe" policy permits normally denied
> + access/execute permission for files in policy on IMA
> + unsupported filesystems. Note this option, as the
> + name implies, is not safe and not recommended for
> + any environments other than testing.
> +
> ima_tcb [IMA] Deprecated. Use ima_policy= instead.
> Load a policy which meets the needs of the Trusted
> Computing Base. This means IMA will measure all
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 43b85a4fb8e8..cddd9dfb01e1 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -169,6 +169,10 @@ static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
> .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
> };
>
> +static struct ima_rule_entry dont_failsafe_rules[] __ro_after_init = {
> + {.action = DONT_FAILSAFE}
> +};
> +
> static LIST_HEAD(ima_default_rules);
> static LIST_HEAD(ima_policy_rules);
> static LIST_HEAD(ima_temp_rules);
> @@ -188,6 +192,7 @@ __setup("ima_tcb", default_measure_policy_setup);
>
> static bool ima_use_appraise_tcb __initdata;
> static bool ima_use_secure_boot __initdata;
> +static bool ima_use_dont_failsafe __initdata;
> static int __init policy_setup(char *str)
> {
> char *p;
> @@ -201,6 +206,10 @@ static int __init policy_setup(char *str)
> ima_use_appraise_tcb = 1;
> else if (strcmp(p, "secure_boot") == 0)
> ima_use_secure_boot = 1;
> + else if (strcmp(p, "fs_unsafe") == 0) {
> + ima_use_dont_failsafe = 1;
> + set_failsafe(0);
> + }
> }
>
> return 1;
> @@ -470,6 +479,9 @@ void __init ima_init_policy(void)
> temp_ima_appraise |= IMA_APPRAISE_POLICY;
> }
>
> + if (ima_use_dont_failsafe)
> + list_add_tail(&dont_failsafe_rules[0].list, &ima_default_rules);
> +
> ima_rules = &ima_default_rules;
> ima_update_policy_flag();
> }
> --
> 2.7.4
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-ima-devel mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linux-ima-devel
--
Thanks,
Dmitry
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 09:34:39
|
On Wed, Jul 26, 2017 at 4:22 PM, Mimi Zohar <zo...@li...> wrote:
> Permit normally denied access/execute permission for files in policy
> on IMA unsupported filesystems. This patch defines the "dont_failsafe"
> policy action rule.
>
> Mimi Zohar <zo...@li...>
>
> ---
> Changelog v3:
> - include dont_failsafe rule when displaying policy
> - fail attempt to add dont_failsafe rule when appending to the policy
>
> Documentation/ABI/testing/ima_policy | 3 ++-
> security/integrity/ima/ima.h | 1 +
> security/integrity/ima/ima_main.c | 11 ++++++++++-
> security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++++++-
> 4 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index e76432b9954d..f271207743e5 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -17,7 +17,8 @@ Description:
>
> rule format: action [condition ...]
>
> - action: measure | dont_measure | appraise | dont_appraise | audit
> + action: measure | dont_meaure | appraise | dont_appraise |
> + audit | dont_failsafe
> condition:= base | lsm [option]
> base: [[func=] [mask=] [fsmagic=] [fsuuid=] [uid=]
> [euid=] [fowner=]]
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index d52b487ad259..c5f34f7c5b0f 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -224,6 +224,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos);
> void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> void ima_policy_stop(struct seq_file *m, void *v);
> int ima_policy_show(struct seq_file *m, void *v);
> +void set_failsafe(bool flag);
>
> /* Appraise integrity measurements */
> #define IMA_APPRAISE_ENFORCE 0x01
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 3941371402ff..664edab0f758 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -38,6 +38,11 @@ int ima_appraise;
> int ima_hash_algo = HASH_ALGO_SHA1;
> static int hash_setup_done;
>
> +static bool ima_failsafe = 1;
> +void set_failsafe(bool flag) {
> + ima_failsafe = flag;
> +}
> +
> static int __init hash_setup(char *str)
> {
> struct ima_template_desc *template_desc = ima_template_desc_current();
> @@ -263,8 +268,12 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> __putname(pathbuf);
> out:
> inode_unlock(inode);
> - if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
> + if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> + if (!ima_failsafe && rc == -EBADF)
> + return 0;
> +
By default IMA is failsaif. ima_failsafe is true.
Return 0 is needed in failsafe mode. right?
But in this logic it will happen if ima_failsafe is false. meaning it
is not failsafe.
Is it a typo?
> return -EACCES;
> + }
> return 0;
> }
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 95209a5f8595..43b85a4fb8e8 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -40,12 +40,14 @@
> #define APPRAISE 0x0004 /* same as IMA_APPRAISE */
> #define DONT_APPRAISE 0x0008
> #define AUDIT 0x0040
> +#define DONT_FAILSAFE 0x0400
>
> #define INVALID_PCR(a) (((a) < 0) || \
> (a) >= (FIELD_SIZEOF(struct integrity_iint_cache, measured_pcrs) * 8))
>
> int ima_policy_flag;
> static int temp_ima_appraise;
> +static bool temp_failsafe = 1;
>
> #define MAX_LSM_RULES 6
> enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
> @@ -513,6 +515,9 @@ void ima_update_policy(void)
> if (ima_rules != policy) {
> ima_policy_flag = 0;
> ima_rules = policy;
> +
> + /* Only update on initial policy replacement, not append */
> + set_failsafe(temp_failsafe);
> }
> ima_update_policy_flag();
> }
> @@ -529,7 +534,7 @@ enum {
> Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
> Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
> Opt_appraise_type, Opt_permit_directio,
> - Opt_pcr
> + Opt_pcr, Opt_dont_failsafe
> };
>
> static match_table_t policy_tokens = {
> @@ -560,6 +565,7 @@ static match_table_t policy_tokens = {
> {Opt_appraise_type, "appraise_type=%s"},
> {Opt_permit_directio, "permit_directio"},
> {Opt_pcr, "pcr=%s"},
> + {Opt_dont_failsafe, "dont_failsafe"},
> {Opt_err, NULL}
> };
>
> @@ -630,6 +636,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> if ((*p == '\0') || (*p == ' ') || (*p == '\t'))
> continue;
> token = match_token(p, policy_tokens, args);
> + if (entry->action == DONT_FAILSAFE) {
> + /* no args permitted, force invalid rule */
> + token = Opt_dont_failsafe;
> + }
> +
> switch (token) {
> case Opt_measure:
> ima_log_string(ab, "action", "measure");
> @@ -671,6 +682,19 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>
> entry->action = AUDIT;
> break;
> + case Opt_dont_failsafe:
> + ima_log_string(ab, "action", "dont_failsafe");
> +
> + if (entry->action != UNKNOWN)
> + result = -EINVAL;
> +
> + /* Permit on initial policy replacement only */
> + if (ima_rules != &ima_policy_rules)
> + temp_failsafe = 0;
> + else
> + result = -EINVAL;
> + entry->action = DONT_FAILSAFE;
> + break;
> case Opt_func:
> ima_log_string(ab, "func", args[0].from);
>
> @@ -949,6 +973,7 @@ void ima_delete_rules(void)
> int i;
>
> temp_ima_appraise = 0;
> + temp_failsafe = 1;
> list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) {
> for (i = 0; i < MAX_LSM_RULES; i++)
> kfree(entry->lsm[i].args_p);
> @@ -1040,6 +1065,8 @@ int ima_policy_show(struct seq_file *m, void *v)
> seq_puts(m, pt(Opt_dont_appraise));
> if (entry->action & AUDIT)
> seq_puts(m, pt(Opt_audit));
> + if (entry->action & DONT_FAILSAFE)
> + seq_puts(m, pt(Opt_dont_failsafe));
>
> seq_puts(m, " ");
>
> --
> 2.7.4
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-ima-devel mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linux-ima-devel
--
Thanks,
Dmitry
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 09:27:30
|
Looks good to me.
On Wed, Jul 26, 2017 at 4:22 PM, Mimi Zohar <zo...@li...> wrote:
> With the new ->integrity_read file_operations method support, files
> opened with the O_DIRECT flag should work properly. This patch
> reverts commit f9b2a735bddd "ima: audit log files opened with O_DIRECT
> flag".
>
> Signed-off-by: Mimi Zohar <zo...@li...>
> ---
> Documentation/ABI/testing/ima_policy | 2 +-
> security/integrity/ima/ima_api.c | 6 ------
> security/integrity/ima/ima_main.c | 5 +----
> security/integrity/ima/ima_policy.c | 8 +-------
> security/integrity/integrity.h | 1 -
> 5 files changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index f271207743e5..441a78e7b87e 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -24,7 +24,7 @@ Description:
> [euid=] [fowner=]]
> lsm: [[subj_user=] [subj_role=] [subj_type=]
> [obj_user=] [obj_role=] [obj_type=]]
> - option: [[appraise_type=]] [permit_directio]
> + option: [[appraise_type=]]
>
> base: func:= [BPRM_CHECK][MMAP_CHECK][FILE_CHECK][MODULE_CHECK]
> [FIRMWARE_CHECK]
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index bbf3ba8bbb09..7bc8e76c06f5 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -210,12 +210,6 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> if (iint->flags & IMA_COLLECTED)
> goto out;
>
> - if (file->f_flags & O_DIRECT) {
> - audit_cause = "failed(directio)";
> - result = -EACCES;
> - goto out;
> - }
> -
> i_version = file_inode(file)->i_version;
> hash.hdr.algo = algo;
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 664edab0f758..9b8ede84337f 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -240,11 +240,8 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
>
> rc = ima_collect_measurement(iint, file, buf, size, hash_algo);
> - if (rc != 0 && rc != -EBADF && rc != -EINVAL) {
> - if (file->f_flags & O_DIRECT)
> - rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES;
> + if (rc != 0 && rc != -EBADF && rc != -EINVAL)
> goto out_digsig;
> - }
>
> if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */
> pathname = ima_d_path(&file->f_path, &pathbuf, filename);
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index cddd9dfb01e1..3b54fb32e837 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -545,7 +545,7 @@ enum {
> Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq,
> Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
> Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
> - Opt_appraise_type, Opt_permit_directio,
> + Opt_appraise_type,
> Opt_pcr, Opt_dont_failsafe
> };
>
> @@ -575,7 +575,6 @@ static match_table_t policy_tokens = {
> {Opt_euid_lt, "euid<%s"},
> {Opt_fowner_lt, "fowner<%s"},
> {Opt_appraise_type, "appraise_type=%s"},
> - {Opt_permit_directio, "permit_directio"},
> {Opt_pcr, "pcr=%s"},
> {Opt_dont_failsafe, "dont_failsafe"},
> {Opt_err, NULL}
> @@ -892,9 +891,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> else
> result = -EINVAL;
> break;
> - case Opt_permit_directio:
> - entry->flags |= IMA_PERMIT_DIRECTIO;
> - break;
> case Opt_pcr:
> if (entry->action != MEASURE) {
> result = -EINVAL;
> @@ -1179,8 +1175,6 @@ 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_PERMIT_DIRECTIO)
> - seq_puts(m, "permit_directio ");
> rcu_read_unlock();
> seq_puts(m, "\n");
> return 0;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index a53e7e4ab06c..790f07e515a7 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -31,7 +31,6 @@
> #define IMA_ACTION_RULE_FLAGS 0x06000000
> #define IMA_DIGSIG 0x01000000
> #define IMA_DIGSIG_REQUIRED 0x02000000
> -#define IMA_PERMIT_DIRECTIO 0x04000000
> #define IMA_NEW_FILE 0x08000000
>
> #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
> --
> 2.7.4
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-ima-devel mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linux-ima-devel
--
Thanks,
Dmitry
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 09:24:47
|
On Wed, Jul 26, 2017 at 4:22 PM, Mimi Zohar <zo...@li...> wrote:
> All files matching a "measure" rule must be included in the IMA
> measurement list, even when the file hash cannot be calculated.
> Similarly, all files matching an "audit" rule must be audited, even when
> the file hash can not be calculated.
>
> The file data hash field contained in the IMA measurement list template
> data will contain 0's instead of the actual file hash digest.
>
> Mimi Zohar <zo...@li...>
>
> ---
> Changelog v4:
> - Based on both -EBADF and -EINVAL
> - clean up ima_collect_measurement()
>
> security/integrity/ima/ima_api.c | 58 +++++++++++++++++++++++----------------
> security/integrity/ima/ima_main.c | 4 +--
> 2 files changed, 37 insertions(+), 25 deletions(-)
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index c2edba8de35e..bbf3ba8bbb09 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -199,37 +199,49 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> struct inode *inode = file_inode(file);
> const char *filename = file->f_path.dentry->d_name.name;
> int result = 0;
> + int length;
> + void *tmpbuf;
> + u64 i_version;
> struct {
> struct ima_digest_data hdr;
> char digest[IMA_MAX_DIGEST_SIZE];
> } hash;
>
> - if (!(iint->flags & IMA_COLLECTED)) {
> - u64 i_version = file_inode(file)->i_version;
> + if (iint->flags & IMA_COLLECTED)
> + goto out;
>
> - if (file->f_flags & O_DIRECT) {
> - audit_cause = "failed(directio)";
> - result = -EACCES;
> - goto out;
> - }
> + if (file->f_flags & O_DIRECT) {
> + audit_cause = "failed(directio)";
> + result = -EACCES;
> + goto out;
> + }
>
> - hash.hdr.algo = algo;
> -
> - result = (!buf) ? ima_calc_file_hash(file, &hash.hdr) :
> - ima_calc_buffer_hash(buf, size, &hash.hdr);
> - if (!result) {
> - int length = sizeof(hash.hdr) + hash.hdr.length;
> - void *tmpbuf = krealloc(iint->ima_hash, length,
> - GFP_NOFS);
> - if (tmpbuf) {
> - iint->ima_hash = tmpbuf;
> - memcpy(iint->ima_hash, &hash, length);
> - iint->version = i_version;
> - iint->flags |= IMA_COLLECTED;
> - } else
> - result = -ENOMEM;
> - }
> + i_version = file_inode(file)->i_version;
> + hash.hdr.algo = algo;
> +
> + /* Initialize hash digest to 0's in case of failure */
> + memset(&hash.digest, 0, sizeof(hash.digest));
> +
> + result = (!buf) ? ima_calc_file_hash(file, &hash.hdr) :
> + ima_calc_buffer_hash(buf, size, &hash.hdr);
> +
> + if (result && result != -EBADF && result != -EINVAL)
> + goto out;
> +
> + length = sizeof(hash.hdr) + hash.hdr.length;
> + tmpbuf = krealloc(iint->ima_hash, length, GFP_NOFS);
> + if (!tmpbuf) {
> + result = -ENOMEM;
> + goto out;
> }
> +
> + iint->ima_hash = tmpbuf;
> + memcpy(iint->ima_hash, &hash, length);
> + iint->version = i_version;
> +
> + /* Possibly temporary failure due to type of read (eg. DAX, O_DIRECT) */
> + if (result != -EBADF && result != -EINVAL)
> + iint->flags |= IMA_COLLECTED;
Result can be other than 0, EBADF and EINVAL here?
It is confusing.. simpler than can be just
if (!result)
iint->flags |= IMA_COLLECTED;
Isn't it?
> out:
> if (result)
> integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 2aebb7984437..3941371402ff 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -235,7 +235,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
>
> rc = ima_collect_measurement(iint, file, buf, size, hash_algo);
> - if (rc != 0) {
> + if (rc != 0 && rc != -EBADF && rc != -EINVAL) {
> if (file->f_flags & O_DIRECT)
> rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES;
> goto out_digsig;
> @@ -247,7 +247,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> if (action & IMA_MEASURE)
> ima_store_measurement(iint, file, pathname,
> xattr_value, xattr_len, pcr);
> - if (action & IMA_APPRAISE_SUBMASK)
> + if ((rc == 0) && (action & IMA_APPRAISE_SUBMASK))
> rc = ima_appraise_measurement(func, iint, file, pathname,
> xattr_value, xattr_len, opened);
> if (action & IMA_AUDIT)
> --
> 2.7.4
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-ima-devel mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linux-ima-devel
--
Thanks,
Dmitry
|
|
From: Mimi Z. <zo...@li...> - 2017-08-21 21:53:43
|
On Mon, 2017-08-21 at 17:50 +0000, Magalhaes, Guilherme (Brazil R&D- CL) wrote: > Hi, > Any feedback about this? > If you agree with the overall proposal I can send a RFC/patch so we > can discuss the details. Only after resolving the outstanding basic IMA namespacing architecture design issues (eg. namespacing IMA-audit) and the code is upstreamed, will I consider anything having to do with namespacing IMA-measurement, including namespacing the IMA policy, or IMA- appraisal. There will be a container micro conference at Linux Plumber Conference (LPC), as well as a namespacing BoF at Linux Security Summit(LSS), where namespacing issues will be discussed. Mimi |
|
From: Magalhaes, G. (B. R&D-CL) <gui...@hp...> - 2017-08-21 17:50:15
|
Hi, Any feedback about this? If you agree with the overall proposal I can send a RFC/patch so we can discuss the details. Thanks. -- Guilherme > -----Original Message----- > From: Magalhaes, Guilherme (Brazil R&D-CL) > Sent: quinta-feira, 10 de agosto de 2017 11:17 > To: 'Mimi Zohar' <zo...@li...> > Cc: lin...@li...; lin...@li... > Subject: RE: [RFC 04/11] ima: add support to namespace securityfs file > > Hi Mimi, > > > The namespacing design will need to support different types of > > environments. For example, depending on the environment, namespaces > > can come and go frequently. In such an environment do we really want > > all the namespace measurements to be included in the system > > measurement list, or should each namespace have its own measurement > > list? Should the namespace measurement policy be the same as the > > system measurement policy? Should the namespace (container) owner be > > allowed to modify their own policy? If a file matches a rule in both > > the namespace and system policies, should the file measurement be in > > both the namespace and the system measurement lists? > > Based on our previous discussions, the IMA policy should be set > independently for different namespaces and each namespace will have a > measurement list. > > Based on that and considering now the issues related to namespacing the > IMA policy interface, this interface to set policy in the host and inside > namespaces could be solved entirely by leveraging the existent policy file. > IMA then would detect the current namespace and assume the rules are > added to that namespace. The root user, even when mapped to a non-root > user outside the namespace, is able to set the namespace policy just like it > is done in the host policy. > > Once the namespace policy is set, the same policy rules should be read back > from the policy file, with the same uids that were set in the rule and that > make sense inside the current user namespace, not the mapped uid to the > 'kernel' uid. This point is important because currently the uids in the IMA > policy rules are mapped to kernel uids at parsing time. > > Another required change for this proposed interface to set the policy is to > allow read/write access to the policy file to all users, since a root user in a > given user namespace may not be a root user outside that user namespace. > The kernel would still block access to the policy file based on the > SYS_ADMIN capability and any changes to the policy would be effective just > for the current namespace. > > The securityfs access inside user namespaces is possible once the user id > mapping is done correctly, the root user, as seen inside the namespace will > be able to mount the securityfs. Again, the read/write access to all users > must be granted for the policy file. > > The same changes could be made to the measurement list interface in > securityfs when each namespace has its own measurement list. > > A management tool in user space might be very helpful to set or read the > policy or measurement list files in the current namespace or its children > namespaces and to map the uid considering the current and target > namespaces. This tool could also relate containers in user space to its IMA > state for policy and appraise mode. > > Does this design solve properly the basic architectural issues only related > to namespacing the interface to set the IMA policy? I have a patch with > these changes ready to be posted. > > -- > Guilherme > > > > -----Original Message----- > > From: Mimi Zohar [mailto:zo...@li...] > > Sent: segunda-feira, 29 de maio de 2017 14:33 > > To: Magalhaes, Guilherme (Brazil R&D-CL) > <gui...@hp...>; > > John Johansen <joh...@ca...>; dmi...@gm... > > Cc: vi...@ze...; jam...@or...; se...@ha...; > > lin...@vg...; lin...@vg...; linux-ima- > > de...@li...; lin...@li...; linux- > > sec...@vg...; ty...@do...; Souza, Joaquim (Brazil > > R&D-ECL) <joa...@hp...>; Edwards, Nigel <nig...@hp...> > > Subject: Re: [RFC 04/11] ima: add support to namespace securityfs file > > > > Hi Guilherme, > > > > (Wow, you should did Cc a lot of people.) > > > > On Thu, 2017-05-25 at 19:04 +0000, Magalhaes, Guilherme (Brazil R&D- > > CL) wrote: > > > Mimi, > > > With the securityfs symlink we would address the case of setting > > > policy inside containers, but we still would need a way to set the > > > IMA policy per namespace outside containers. So, the current > > > proposed interface would address the latter case. > > > As an alternative to symlinks, taking this patch set as base, and > > > still considering setting policy inside containers (or inside > > > namespaces in general), it is possible to bind mount the securityfs > > > files into the containers, but it would be needed to prevent > > > read/write access to the namespaced IMA policy files for processes > > > not running on the same namespace. > > > > > > These mechanisms would not require a change in the proposed design. > > > Do you think these mechanisms are enough for the flexibility you > > > asked? > > > > I'm really sorry Guileherme, but as I previously explained, IMA has > > many aspects to it - the original file measurements (IMA-measurement), > > file hash/signature appraisal (IMA-appraisal), and file audit messages > > (IMA-audit) used for security analytics/forensics, not the file system > > auditing. To namespace IMA properly requires addressing some > > underlying problems - securityfs, root privilege required for writing > > security xattrs, per namespace IMA keyring - to name a few. > > > > I understand wanting to namespace the appraisal aspect first, but when > > you asked me if anyone else is working on namespacing IMA at the > > moment, I suggested starting with the IMA-audit aspect for a > > reason. By beginning with the IMA-audit aspect, we could ignore, at > > least for the time being, some of these underlying problems, but > > define the basic namespacing architecture. By jumping to namespacing > > the appraisal aspect, you've simply avoided addressing the IMA > > namespacing architecture issues, which need to be addressed. > > > > Here are some, not by any means all, of the issues with namespacing > > IMA. > > > > > > - IMA-measurements: > > > > The namespacing design will need to support different types of > > environments. For example, depending on the environment, namespaces > > can come and go frequently. In such an environment do we really want > > all the namespace measurements to be included in the system > > measurement list, or should each namespace have its own measurement > > list? Should the namespace measurement policy be the same as the > > system measurement policy? Should the namespace (container) owner be > > allowed to modify their own policy? If a file matches a rule in both > > the namespace and system policies, should the file measurement be in > > both the namespace and the system measurement lists? > > > > > > - IMA-appraisal > > > > Assuming the IMA appraisal policy requires file signatures, signatures > > are verified based on the keys on the IMA keyring. When discussing > > namespacing IMA-appraisal, we might want different sets of keys in > > different namespaces. This requires separate keyrings for each > > namespace. In some use cases, we might want to include the keys on > > the system IMA keyring, while in other use cases we might not. > > > > Even if real root initially installs the files with their file > > signatures, stored as extended attributes, how will software be > > updated, as writing file signatures requires root > > privilege?Capabilities has recently added support to permit root in > > the namespace to write security.capability. Similarly when > > namespacing IMA, root in the namespace needs the ability to write > > security.ima. > > > > > > - IMA-audit: > > > > The IMA-audit messages can augment other file system security > > information used in security analytics/forensics. This information > > should be on a per namespace basis, meaning that each time a new file > > is accessed/executed, there needs to be a separate audit message, even > > if a message already exists in another namespace. Maintaining and > > cleaning up this per namespace cache information, allows development > > of the IMA namespace architecture independently of other issues. > > > > My original suggestion stands. Start with namespacing IMA- > > audit. Afterwards resolve the securityfs issues needed for > > namespacing IMA-measurement, and subsequently resolve the keyring and > > xattr issues for namespacing IMA-appraisal. Although other subsystems > > have already addressed some of the issues listed here, the advice to > > start with IMA-audit is still valid. > > > > Small incremental change does not imply without an overall design, but > > an overall design which is broken up in such a way (to ease review) > > that builds upon the previous change. > > > > As both Apparmor and IMA use securityfs for policy, it would be nice > > if their method for loading namespace policies would be similar too. > > > > Mimi |
|
From: Gilad Ben-Y. <gi...@be...> - 2017-08-21 14:09:58
|
The code sample is waiting for an async. crypto op completion.
Adapt sample to use the new generic infrastructure to do the same.
This also fixes a possible data coruption bug created by the
use of wait_for_completion_interruptible() without dealing
correctly with an interrupt aborting the wait prior to the
async op finishing.
Signed-off-by: Gilad Ben-Yossef <gi...@be...>
---
Documentation/crypto/api-samples.rst | 52 +++++++-----------------------------
1 file changed, 10 insertions(+), 42 deletions(-)
diff --git a/Documentation/crypto/api-samples.rst b/Documentation/crypto/api-samples.rst
index 2531948..006827e 100644
--- a/Documentation/crypto/api-samples.rst
+++ b/Documentation/crypto/api-samples.rst
@@ -7,59 +7,27 @@ Code Example For Symmetric Key Cipher Operation
::
- struct tcrypt_result {
- struct completion completion;
- int err;
- };
-
/* tie all data structures together */
struct skcipher_def {
struct scatterlist sg;
struct crypto_skcipher *tfm;
struct skcipher_request *req;
- struct tcrypt_result result;
+ struct crypto_wait wait;
};
- /* Callback function */
- static void test_skcipher_cb(struct crypto_async_request *req, int error)
- {
- struct tcrypt_result *result = req->data;
-
- if (error == -EINPROGRESS)
- return;
- result->err = error;
- complete(&result->completion);
- pr_info("Encryption finished successfully\n");
- }
-
/* Perform cipher operation */
static unsigned int test_skcipher_encdec(struct skcipher_def *sk,
int enc)
{
- int rc = 0;
+ int rc;
if (enc)
- rc = crypto_skcipher_encrypt(sk->req);
+ rc = crypto_wait_req(crypto_skcipher_encrypt(sk->req), &sk->wait);
else
- rc = crypto_skcipher_decrypt(sk->req);
-
- switch (rc) {
- case 0:
- break;
- case -EINPROGRESS:
- case -EBUSY:
- rc = wait_for_completion_interruptible(
- &sk->result.completion);
- if (!rc && !sk->result.err) {
- reinit_completion(&sk->result.completion);
- break;
- }
- default:
- pr_info("skcipher encrypt returned with %d result %d\n",
- rc, sk->result.err);
- break;
- }
- init_completion(&sk->result.completion);
+ rc = crypto_wait_req(crypto_skcipher_decrypt(sk->req), &sk->wait);
+
+ if (rc)
+ pr_info("skcipher encrypt returned with result %d\n", rc);
return rc;
}
@@ -89,8 +57,8 @@ Code Example For Symmetric Key Cipher Operation
}
skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
- test_skcipher_cb,
- &sk.result);
+ crypto_req_done,
+ &sk.wait);
/* AES 256 with random key */
get_random_bytes(&key, 32);
@@ -122,7 +90,7 @@ Code Example For Symmetric Key Cipher Operation
/* We encrypt one block */
sg_init_one(&sk.sg, scratchpad, 16);
skcipher_request_set_crypt(req, &sk.sg, &sk.sg, 16, ivdata);
- init_completion(&sk.result.completion);
+ crypto_init_wait(&sk.wait);
/* encrypt data */
ret = test_skcipher_encdec(&sk, 1);
--
2.1.4
|
|
From: Gilad Ben-Y. <gi...@be...> - 2017-08-21 14:09:46
|
The mediatek driver starts several async crypto ops and waits for their
completions. Move it over to generic code doing the same.
Signed-off-by: Gilad Ben-Yossef <gi...@be...>
Acked-by: Ryder Lee <ryd...@me...>
---
drivers/crypto/mediatek/mtk-aes.c | 31 +++++--------------------------
1 file changed, 5 insertions(+), 26 deletions(-)
diff --git a/drivers/crypto/mediatek/mtk-aes.c b/drivers/crypto/mediatek/mtk-aes.c
index 9e845e8..e2c7c95 100644
--- a/drivers/crypto/mediatek/mtk-aes.c
+++ b/drivers/crypto/mediatek/mtk-aes.c
@@ -137,11 +137,6 @@ struct mtk_aes_gcm_ctx {
struct crypto_skcipher *ctr;
};
-struct mtk_aes_gcm_setkey_result {
- int err;
- struct completion completion;
-};
-
struct mtk_aes_drv {
struct list_head dev_list;
/* Device list lock */
@@ -936,17 +931,6 @@ static int mtk_aes_gcm_crypt(struct aead_request *req, u64 mode)
&req->base);
}
-static void mtk_gcm_setkey_done(struct crypto_async_request *req, int err)
-{
- struct mtk_aes_gcm_setkey_result *result = req->data;
-
- if (err == -EINPROGRESS)
- return;
-
- result->err = err;
- complete(&result->completion);
-}
-
/*
* Because of the hardware limitation, we need to pre-calculate key(H)
* for the GHASH operation. The result of the encryption operation
@@ -962,7 +946,7 @@ static int mtk_aes_gcm_setkey(struct crypto_aead *aead, const u8 *key,
u32 hash[4];
u8 iv[8];
- struct mtk_aes_gcm_setkey_result result;
+ struct crypto_wait wait;
struct scatterlist sg[1];
struct skcipher_request req;
@@ -1002,22 +986,17 @@ static int mtk_aes_gcm_setkey(struct crypto_aead *aead, const u8 *key,
if (!data)
return -ENOMEM;
- init_completion(&data->result.completion);
+ crypto_init_wait(&data->wait);
sg_init_one(data->sg, &data->hash, AES_BLOCK_SIZE);
skcipher_request_set_tfm(&data->req, ctr);
skcipher_request_set_callback(&data->req, CRYPTO_TFM_REQ_MAY_SLEEP |
CRYPTO_TFM_REQ_MAY_BACKLOG,
- mtk_gcm_setkey_done, &data->result);
+ crypto_req_done, &data->wait);
skcipher_request_set_crypt(&data->req, data->sg, data->sg,
AES_BLOCK_SIZE, data->iv);
- err = crypto_skcipher_encrypt(&data->req);
- if (err == -EINPROGRESS || err == -EBUSY) {
- err = wait_for_completion_interruptible(
- &data->result.completion);
- if (!err)
- err = data->result.err;
- }
+ err = crypto_wait_req(crypto_skcipher_encrypt(&data->req),
+ &data->wait);
if (err)
goto out;
--
2.1.4
|
116 messages has been excluded from this view by a project administrator.
