* [PATCH] ima: Make a copy of sig and digest in asymmetric_verify()
@ 2022-11-04 12:20 Roberto Sassu
2022-11-21 13:39 ` Roberto Sassu
2022-11-22 19:39 ` Mimi Zohar
0 siblings, 2 replies; 9+ messages in thread
From: Roberto Sassu @ 2022-11-04 12:20 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, paul, jmorris, serge
Cc: linux-integrity, linux-security-module, linux-kernel,
Roberto Sassu, stable
From: Roberto Sassu <roberto.sassu@huawei.com>
Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
mapping") requires that both the signature and the digest resides in the
linear mapping area.
However, more recently commit ba14a194a434c ("fork: Add generic vmalloced
stack support"), made it possible to move the stack in the vmalloc area,
which could make the requirement of the first commit not satisfied anymore.
If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered:
[ 467.077359] kernel BUG at include/linux/scatterlist.h:163!
[ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
[...]
[ 467.095225] Call Trace:
[ 467.096088] <TASK>
[ 467.096928] ? rcu_read_lock_held_common+0xe/0x50
[ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70
[ 467.098123] ? trace_hardirqs_on+0x2c/0xd0
[ 467.098647] ? public_key_verify_signature+0x470/0x470
[ 467.099237] asymmetric_verify+0x14c/0x300
[ 467.099869] evm_verify_hmac+0x245/0x360
[ 467.100391] evm_inode_setattr+0x43/0x190
The failure happens only for the digest, as the pointer comes from the
stack, and not for the signature, which instead was allocated by
vfs_getxattr_alloc().
Fix this by making a copy of both in asymmetric_verify(), so that the
linear mapping requirement is always satisfied, regardless of the caller.
Cc: stable@vger.kernel.org # 4.9.x
Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
security/integrity/digsig_asymmetric.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
index 895f4b9ce8c6..635238d5c7fe 100644
--- a/security/integrity/digsig_asymmetric.c
+++ b/security/integrity/digsig_asymmetric.c
@@ -122,11 +122,26 @@ int asymmetric_verify(struct key *keyring, const char *sig,
goto out;
}
- pks.digest = (u8 *)data;
+ pks.digest = kmemdup(data, datalen, GFP_KERNEL);
+ if (!pks.digest) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
pks.digest_size = datalen;
- pks.s = hdr->sig;
+
+ pks.s = kmemdup(hdr->sig, siglen, GFP_KERNEL);
+ if (!pks.s) {
+ kfree(pks.digest);
+ ret = -ENOMEM;
+ goto out;
+ }
+
pks.s_size = siglen;
+
ret = verify_signature(key, &pks);
+ kfree(pks.digest);
+ kfree(pks.s);
out:
key_put(key);
pr_debug("%s() = %d\n", __func__, ret);
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] ima: Make a copy of sig and digest in asymmetric_verify() 2022-11-04 12:20 [PATCH] ima: Make a copy of sig and digest in asymmetric_verify() Roberto Sassu @ 2022-11-21 13:39 ` Roberto Sassu 2022-11-22 19:39 ` Mimi Zohar 1 sibling, 0 replies; 9+ messages in thread From: Roberto Sassu @ 2022-11-21 13:39 UTC (permalink / raw) To: zohar, dmitry.kasatkin, paul, jmorris, serge Cc: linux-integrity, linux-security-module, linux-kernel, Roberto Sassu, stable On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > mapping") requires that both the signature and the digest resides in the > linear mapping area. > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > stack support"), made it possible to move the stack in the vmalloc area, > which could make the requirement of the first commit not satisfied anymore. > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: Hi Mimi did you have the chance to have a look at this patch? Thanks Roberto > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > [...] > > [ 467.095225] Call Trace: > [ 467.096088] <TASK> > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > [ 467.099237] asymmetric_verify+0x14c/0x300 > [ 467.099869] evm_verify_hmac+0x245/0x360 > [ 467.100391] evm_inode_setattr+0x43/0x190 > > The failure happens only for the digest, as the pointer comes from the > stack, and not for the signature, which instead was allocated by > vfs_getxattr_alloc(). > > Fix this by making a copy of both in asymmetric_verify(), so that the > linear mapping requirement is always satisfied, regardless of the caller. > > Cc: stable@vger.kernel.org # 4.9.x > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > security/integrity/digsig_asymmetric.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c > index 895f4b9ce8c6..635238d5c7fe 100644 > --- a/security/integrity/digsig_asymmetric.c > +++ b/security/integrity/digsig_asymmetric.c > @@ -122,11 +122,26 @@ int asymmetric_verify(struct key *keyring, const char *sig, > goto out; > } > > - pks.digest = (u8 *)data; > + pks.digest = kmemdup(data, datalen, GFP_KERNEL); > + if (!pks.digest) { > + ret = -ENOMEM; > + goto out; > + } > + > pks.digest_size = datalen; > - pks.s = hdr->sig; > + > + pks.s = kmemdup(hdr->sig, siglen, GFP_KERNEL); > + if (!pks.s) { > + kfree(pks.digest); > + ret = -ENOMEM; > + goto out; > + } > + > pks.s_size = siglen; > + > ret = verify_signature(key, &pks); > + kfree(pks.digest); > + kfree(pks.s); > out: > key_put(key); > pr_debug("%s() = %d\n", __func__, ret); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ima: Make a copy of sig and digest in asymmetric_verify() 2022-11-04 12:20 [PATCH] ima: Make a copy of sig and digest in asymmetric_verify() Roberto Sassu 2022-11-21 13:39 ` Roberto Sassu @ 2022-11-22 19:39 ` Mimi Zohar 2022-11-23 12:56 ` Roberto Sassu 1 sibling, 1 reply; 9+ messages in thread From: Mimi Zohar @ 2022-11-22 19:39 UTC (permalink / raw) To: Roberto Sassu, dmitry.kasatkin, paul, jmorris, serge Cc: linux-integrity, linux-security-module, linux-kernel, Roberto Sassu, stable Hi Roberto, On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > mapping") requires that both the signature and the digest resides in the > linear mapping area. > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > stack support"), made it possible to move the stack in the vmalloc area, > which could make the requirement of the first commit not satisfied anymore. > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: ^CONFIG_DEBUG_SG > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > [...] > > [ 467.095225] Call Trace: > [ 467.096088] <TASK> > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > [ 467.099237] asymmetric_verify+0x14c/0x300 > [ 467.099869] evm_verify_hmac+0x245/0x360 > [ 467.100391] evm_inode_setattr+0x43/0x190 > > The failure happens only for the digest, as the pointer comes from the > stack, and not for the signature, which instead was allocated by > vfs_getxattr_alloc(). Only after enabling CONFIG_DEBUG_SG does EVM fail. > > Fix this by making a copy of both in asymmetric_verify(), so that the > linear mapping requirement is always satisfied, regardless of the caller. As only EVM is affected, it would make more sense to limit the change to EVM. -- thanks, Mimi ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ima: Make a copy of sig and digest in asymmetric_verify() 2022-11-22 19:39 ` Mimi Zohar @ 2022-11-23 12:56 ` Roberto Sassu 2022-11-23 13:40 ` Mimi Zohar 0 siblings, 1 reply; 9+ messages in thread From: Roberto Sassu @ 2022-11-23 12:56 UTC (permalink / raw) To: Mimi Zohar, dmitry.kasatkin, paul, jmorris, serge Cc: linux-integrity, linux-security-module, linux-kernel, Roberto Sassu, stable On Tue, 2022-11-22 at 14:39 -0500, Mimi Zohar wrote: > Hi Roberto, > > On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > mapping") requires that both the signature and the digest resides in the > > linear mapping area. > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > stack support"), made it possible to move the stack in the vmalloc area, > > which could make the requirement of the first commit not satisfied anymore. > > > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: > > ^CONFIG_DEBUG_SG > > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > > > [...] > > > > [ 467.095225] Call Trace: > > [ 467.096088] <TASK> > > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > > [ 467.099237] asymmetric_verify+0x14c/0x300 > > [ 467.099869] evm_verify_hmac+0x245/0x360 > > [ 467.100391] evm_inode_setattr+0x43/0x190 > > > > The failure happens only for the digest, as the pointer comes from the > > stack, and not for the signature, which instead was allocated by > > vfs_getxattr_alloc(). > > Only after enabling CONFIG_DEBUG_SG does EVM fail. > > > Fix this by making a copy of both in asymmetric_verify(), so that the > > linear mapping requirement is always satisfied, regardless of the caller. > > As only EVM is affected, it would make more sense to limit the change > to EVM. I found another occurrence: static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, struct evm_ima_xattr_data *xattr_value, int xattr_len, enum integrity_status *status, const char **cause) { [...] rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, (const char *)xattr_value, xattr_len, hash.digest, hash.hdr.length); Should I do two patches? Thanks Roberto ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ima: Make a copy of sig and digest in asymmetric_verify() 2022-11-23 12:56 ` Roberto Sassu @ 2022-11-23 13:40 ` Mimi Zohar 2022-11-23 13:49 ` Roberto Sassu 0 siblings, 1 reply; 9+ messages in thread From: Mimi Zohar @ 2022-11-23 13:40 UTC (permalink / raw) To: Roberto Sassu, dmitry.kasatkin, paul, jmorris, serge Cc: linux-integrity, linux-security-module, linux-kernel, Roberto Sassu, stable On Wed, 2022-11-23 at 13:56 +0100, Roberto Sassu wrote: > On Tue, 2022-11-22 at 14:39 -0500, Mimi Zohar wrote: > > Hi Roberto, > > > > On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > mapping") requires that both the signature and the digest resides in the > > > linear mapping area. > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > stack support"), made it possible to move the stack in the vmalloc area, > > > which could make the requirement of the first commit not satisfied anymore. > > > > > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: > > > > ^CONFIG_DEBUG_SG > > > > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > > > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > > > > > [...] > > > > > > [ 467.095225] Call Trace: > > > [ 467.096088] <TASK> > > > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > > > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > > > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > > > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > > > [ 467.099237] asymmetric_verify+0x14c/0x300 > > > [ 467.099869] evm_verify_hmac+0x245/0x360 > > > [ 467.100391] evm_inode_setattr+0x43/0x190 > > > > > > The failure happens only for the digest, as the pointer comes from the > > > stack, and not for the signature, which instead was allocated by > > > vfs_getxattr_alloc(). > > > > Only after enabling CONFIG_DEBUG_SG does EVM fail. > > > > > Fix this by making a copy of both in asymmetric_verify(), so that the > > > linear mapping requirement is always satisfied, regardless of the caller. > > > > As only EVM is affected, it would make more sense to limit the change > > to EVM. > > I found another occurrence: > > static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > struct evm_ima_xattr_data *xattr_value, int xattr_len, > enum integrity_status *status, const char **cause) > { > > [...] > > rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > (const char *)xattr_value, > xattr_len, hash.digest, > hash.hdr.length); > > Should I do two patches? I'm just not getting it. Why did you enable CONFIG_DEBUG_SIG? Were you testing random kernel configs? Are you actually seeing signature verifications errors without it enabled? Or is it causing other problems? Is the "BUG_ON" still needed? If you're going to fix the EVM and IMA callers, then make them separate patches. -- thanks, Mimi ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ima: Make a copy of sig and digest in asymmetric_verify() 2022-11-23 13:40 ` Mimi Zohar @ 2022-11-23 13:49 ` Roberto Sassu 2022-11-30 14:41 ` Roberto Sassu 0 siblings, 1 reply; 9+ messages in thread From: Roberto Sassu @ 2022-11-23 13:49 UTC (permalink / raw) To: Mimi Zohar, dmitry.kasatkin, paul, jmorris, serge, rusty, axboe Cc: linux-integrity, linux-security-module, linux-kernel, Roberto Sassu, stable On Wed, 2022-11-23 at 08:40 -0500, Mimi Zohar wrote: > On Wed, 2022-11-23 at 13:56 +0100, Roberto Sassu wrote: > > On Tue, 2022-11-22 at 14:39 -0500, Mimi Zohar wrote: > > > Hi Roberto, > > > > > > On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > mapping") requires that both the signature and the digest resides in the > > > > linear mapping area. > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > stack support"), made it possible to move the stack in the vmalloc area, > > > > which could make the requirement of the first commit not satisfied anymore. > > > > > > > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: > > > > > > ^CONFIG_DEBUG_SG > > > > > > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > > > > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > > > > > > > [...] > > > > > > > > [ 467.095225] Call Trace: > > > > [ 467.096088] <TASK> > > > > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > > > > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > > > > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > > > > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > > > > [ 467.099237] asymmetric_verify+0x14c/0x300 > > > > [ 467.099869] evm_verify_hmac+0x245/0x360 > > > > [ 467.100391] evm_inode_setattr+0x43/0x190 > > > > > > > > The failure happens only for the digest, as the pointer comes from the > > > > stack, and not for the signature, which instead was allocated by > > > > vfs_getxattr_alloc(). > > > > > > Only after enabling CONFIG_DEBUG_SG does EVM fail. > > > > > > > Fix this by making a copy of both in asymmetric_verify(), so that the > > > > linear mapping requirement is always satisfied, regardless of the caller. > > > > > > As only EVM is affected, it would make more sense to limit the change > > > to EVM. > > > > I found another occurrence: > > > > static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > > struct evm_ima_xattr_data *xattr_value, int xattr_len, > > enum integrity_status *status, const char **cause) > > { > > > > [...] > > > > rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > > (const char *)xattr_value, > > xattr_len, hash.digest, > > hash.hdr.length); > > > > Should I do two patches? > > I'm just not getting it. Why did you enable CONFIG_DEBUG_SIG? Were > you testing random kernel configs? Are you actually seeing signature > verifications errors without it enabled? Or is it causing other > problems? Is the "BUG_ON" still needed? When I test patches, I tend to enable more debugging options. To be honest, I didn't check if there is any issue without enabling CONFIG_DEBUG_SG. I thought that if there is a linear mapping requirement, that should be satisfied regardless of whether the debugging option is enabled or not. + Rusty, Jens for explanations. > If you're going to fix the EVM and IMA callers, then make them separate > patches. Ok. Thanks Roberto ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ima: Make a copy of sig and digest in asymmetric_verify() 2022-11-23 13:49 ` Roberto Sassu @ 2022-11-30 14:41 ` Roberto Sassu 2022-11-30 16:22 ` Mimi Zohar 0 siblings, 1 reply; 9+ messages in thread From: Roberto Sassu @ 2022-11-30 14:41 UTC (permalink / raw) To: Mimi Zohar, dmitry.kasatkin, paul, jmorris, serge, rusty, axboe Cc: linux-integrity, linux-security-module, linux-kernel, Roberto Sassu, stable On Wed, 2022-11-23 at 14:49 +0100, Roberto Sassu wrote: > On Wed, 2022-11-23 at 08:40 -0500, Mimi Zohar wrote: > > On Wed, 2022-11-23 at 13:56 +0100, Roberto Sassu wrote: > > > On Tue, 2022-11-22 at 14:39 -0500, Mimi Zohar wrote: > > > > Hi Roberto, > > > > > > > > On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > > mapping") requires that both the signature and the digest resides in the > > > > > linear mapping area. > > > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > > stack support"), made it possible to move the stack in the vmalloc area, > > > > > which could make the requirement of the first commit not satisfied anymore. > > > > > > > > > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: > > > > > > > > ^CONFIG_DEBUG_SG > > > > > > > > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > > > > > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > > > > > > > > > [...] > > > > > > > > > > [ 467.095225] Call Trace: > > > > > [ 467.096088] <TASK> > > > > > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > > > > > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > > > > > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > > > > > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > > > > > [ 467.099237] asymmetric_verify+0x14c/0x300 > > > > > [ 467.099869] evm_verify_hmac+0x245/0x360 > > > > > [ 467.100391] evm_inode_setattr+0x43/0x190 > > > > > > > > > > The failure happens only for the digest, as the pointer comes from the > > > > > stack, and not for the signature, which instead was allocated by > > > > > vfs_getxattr_alloc(). > > > > > > > > Only after enabling CONFIG_DEBUG_SG does EVM fail. > > > > > > > > > Fix this by making a copy of both in asymmetric_verify(), so that the > > > > > linear mapping requirement is always satisfied, regardless of the caller. > > > > > > > > As only EVM is affected, it would make more sense to limit the change > > > > to EVM. > > > > > > I found another occurrence: > > > > > > static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > > > struct evm_ima_xattr_data *xattr_value, int xattr_len, > > > enum integrity_status *status, const char **cause) > > > { > > > > > > [...] > > > > > > rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > > > (const char *)xattr_value, > > > xattr_len, hash.digest, > > > hash.hdr.length); > > > > > > Should I do two patches? > > > > I'm just not getting it. Why did you enable CONFIG_DEBUG_SIG? Were > > you testing random kernel configs? Are you actually seeing signature > > verifications errors without it enabled? Or is it causing other > > problems? Is the "BUG_ON" still needed? > > When I test patches, I tend to enable more debugging options. > > To be honest, I didn't check if there is any issue without enabling > CONFIG_DEBUG_SG. I thought that if there is a linear mapping > requirement, that should be satisfied regardless of whether the > debugging option is enabled or not. > > + Rusty, Jens for explanations. Trying to answer the question, with the help of an old discussion: https://groups.google.com/g/linux.kernel/c/dpIoiY_qSGc sg_set_buf() calls virt_to_page() to get the page to start from. But if the buffer spans in two pages, that would not work in the vmalloc area, since there is no guarantee that the next page is adjiacent. For small areas, much smaller than the page size, it is unlikely that the situation above would happen. So, integrity_digsig_verify() will likely succeeed. Although it is possible that it fails if there are data in the next page. Roberto ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ima: Make a copy of sig and digest in asymmetric_verify() 2022-11-30 14:41 ` Roberto Sassu @ 2022-11-30 16:22 ` Mimi Zohar 2022-11-30 16:24 ` Roberto Sassu 0 siblings, 1 reply; 9+ messages in thread From: Mimi Zohar @ 2022-11-30 16:22 UTC (permalink / raw) To: Roberto Sassu, dmitry.kasatkin, paul, jmorris, serge, rusty, axboe Cc: linux-integrity, linux-security-module, linux-kernel, Roberto Sassu, stable On Wed, 2022-11-30 at 15:41 +0100, Roberto Sassu wrote: > On Wed, 2022-11-23 at 14:49 +0100, Roberto Sassu wrote: > > On Wed, 2022-11-23 at 08:40 -0500, Mimi Zohar wrote: > > > On Wed, 2022-11-23 at 13:56 +0100, Roberto Sassu wrote: > > > > On Tue, 2022-11-22 at 14:39 -0500, Mimi Zohar wrote: > > > > > Hi Roberto, > > > > > > > > > > On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > > > mapping") requires that both the signature and the digest resides in the > > > > > > linear mapping area. > > > > > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > > > stack support"), made it possible to move the stack in the vmalloc area, > > > > > > which could make the requirement of the first commit not satisfied anymore. > > > > > > > > > > > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: > > > > > > > > > > ^CONFIG_DEBUG_SG > > > > > > > > > > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > > > > > > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > > > > > > > > > > > [...] > > > > > > > > > > > > [ 467.095225] Call Trace: > > > > > > [ 467.096088] <TASK> > > > > > > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > > > > > > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > > > > > > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > > > > > > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > > > > > > [ 467.099237] asymmetric_verify+0x14c/0x300 > > > > > > [ 467.099869] evm_verify_hmac+0x245/0x360 > > > > > > [ 467.100391] evm_inode_setattr+0x43/0x190 > > > > > > > > > > > > The failure happens only for the digest, as the pointer comes from the > > > > > > stack, and not for the signature, which instead was allocated by > > > > > > vfs_getxattr_alloc(). > > > > > > > > > > Only after enabling CONFIG_DEBUG_SG does EVM fail. > > > > > > > > > > > Fix this by making a copy of both in asymmetric_verify(), so that the > > > > > > linear mapping requirement is always satisfied, regardless of the caller. > > > > > > > > > > As only EVM is affected, it would make more sense to limit the change > > > > > to EVM. > > > > > > > > I found another occurrence: > > > > > > > > static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > > > > struct evm_ima_xattr_data *xattr_value, int xattr_len, > > > > enum integrity_status *status, const char **cause) > > > > { > > > > > > > > [...] > > > > > > > > rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > > > > (const char *)xattr_value, > > > > xattr_len, hash.digest, > > > > hash.hdr.length); > > > > > > > > Should I do two patches? > > > > > > I'm just not getting it. Why did you enable CONFIG_DEBUG_SIG? Were > > > you testing random kernel configs? Are you actually seeing signature > > > verifications errors without it enabled? Or is it causing other > > > problems? Is the "BUG_ON" still needed? > > > > When I test patches, I tend to enable more debugging options. > > > > To be honest, I didn't check if there is any issue without enabling > > CONFIG_DEBUG_SG. I thought that if there is a linear mapping > > requirement, that should be satisfied regardless of whether the > > debugging option is enabled or not. > > > > + Rusty, Jens for explanations. > > Trying to answer the question, with the help of an old discussion: > > https://groups.google.com/g/linux.kernel/c/dpIoiY_qSGc > > sg_set_buf() calls virt_to_page() to get the page to start from. But if > the buffer spans in two pages, that would not work in the vmalloc area, > since there is no guarantee that the next page is adjiacent. > > For small areas, much smaller than the page size, it is unlikely that > the situation above would happen. So, integrity_digsig_verify() will > likely succeeed. Although it is possible that it fails if there are > data in the next page. Thanks, Roberto. Confirmed that as the patch description indicates, without CONFIG_VMAP_STACK configured and with CONFIG_DEBUG_SG enabled there isn't a bug. Does it make sense to limit this change to just CONFIG_VMAP_STACK? -- thanks, Mimi ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ima: Make a copy of sig and digest in asymmetric_verify() 2022-11-30 16:22 ` Mimi Zohar @ 2022-11-30 16:24 ` Roberto Sassu 0 siblings, 0 replies; 9+ messages in thread From: Roberto Sassu @ 2022-11-30 16:24 UTC (permalink / raw) To: Mimi Zohar, dmitry.kasatkin, paul, jmorris, serge, rusty, axboe Cc: linux-integrity, linux-security-module, linux-kernel, Roberto Sassu, stable On Wed, 2022-11-30 at 11:22 -0500, Mimi Zohar wrote: > On Wed, 2022-11-30 at 15:41 +0100, Roberto Sassu wrote: > > On Wed, 2022-11-23 at 14:49 +0100, Roberto Sassu wrote: > > > On Wed, 2022-11-23 at 08:40 -0500, Mimi Zohar wrote: > > > > On Wed, 2022-11-23 at 13:56 +0100, Roberto Sassu wrote: > > > > > On Tue, 2022-11-22 at 14:39 -0500, Mimi Zohar wrote: > > > > > > Hi Roberto, > > > > > > > > > > > > On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote: > > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > > > > mapping") requires that both the signature and the digest resides in the > > > > > > > linear mapping area. > > > > > > > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > > > > stack support"), made it possible to move the stack in the vmalloc area, > > > > > > > which could make the requirement of the first commit not satisfied anymore. > > > > > > > > > > > > > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered: > > > > > > > > > > > > ^CONFIG_DEBUG_SG > > > > > > > > > > > > > [ 467.077359] kernel BUG at include/linux/scatterlist.h:163! > > > > > > > [ 467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > [ 467.095225] Call Trace: > > > > > > > [ 467.096088] <TASK> > > > > > > > [ 467.096928] ? rcu_read_lock_held_common+0xe/0x50 > > > > > > > [ 467.097569] ? rcu_read_lock_sched_held+0x13/0x70 > > > > > > > [ 467.098123] ? trace_hardirqs_on+0x2c/0xd0 > > > > > > > [ 467.098647] ? public_key_verify_signature+0x470/0x470 > > > > > > > [ 467.099237] asymmetric_verify+0x14c/0x300 > > > > > > > [ 467.099869] evm_verify_hmac+0x245/0x360 > > > > > > > [ 467.100391] evm_inode_setattr+0x43/0x190 > > > > > > > > > > > > > > The failure happens only for the digest, as the pointer comes from the > > > > > > > stack, and not for the signature, which instead was allocated by > > > > > > > vfs_getxattr_alloc(). > > > > > > > > > > > > Only after enabling CONFIG_DEBUG_SG does EVM fail. > > > > > > > > > > > > > Fix this by making a copy of both in asymmetric_verify(), so that the > > > > > > > linear mapping requirement is always satisfied, regardless of the caller. > > > > > > > > > > > > As only EVM is affected, it would make more sense to limit the change > > > > > > to EVM. > > > > > > > > > > I found another occurrence: > > > > > > > > > > static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > > > > > struct evm_ima_xattr_data *xattr_value, int xattr_len, > > > > > enum integrity_status *status, const char **cause) > > > > > { > > > > > > > > > > [...] > > > > > > > > > > rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > > > > > (const char *)xattr_value, > > > > > xattr_len, hash.digest, > > > > > hash.hdr.length); > > > > > > > > > > Should I do two patches? > > > > > > > > I'm just not getting it. Why did you enable CONFIG_DEBUG_SIG? Were > > > > you testing random kernel configs? Are you actually seeing signature > > > > verifications errors without it enabled? Or is it causing other > > > > problems? Is the "BUG_ON" still needed? > > > > > > When I test patches, I tend to enable more debugging options. > > > > > > To be honest, I didn't check if there is any issue without enabling > > > CONFIG_DEBUG_SG. I thought that if there is a linear mapping > > > requirement, that should be satisfied regardless of whether the > > > debugging option is enabled or not. > > > > > > + Rusty, Jens for explanations. > > > > Trying to answer the question, with the help of an old discussion: > > > > https://groups.google.com/g/linux.kernel/c/dpIoiY_qSGc > > > > sg_set_buf() calls virt_to_page() to get the page to start from. But if > > the buffer spans in two pages, that would not work in the vmalloc area, > > since there is no guarantee that the next page is adjiacent. > > > > For small areas, much smaller than the page size, it is unlikely that > > the situation above would happen. So, integrity_digsig_verify() will > > likely succeeed. Although it is possible that it fails if there are > > data in the next page. > > Thanks, Roberto. Confirmed that as the patch description indicates, > without CONFIG_VMAP_STACK configured and with CONFIG_DEBUG_SG enabled > there isn't a bug. Does it make sense to limit this change to just > CONFIG_VMAP_STACK? Yes, I agree. Roberto ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-30 16:25 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-04 12:20 [PATCH] ima: Make a copy of sig and digest in asymmetric_verify() Roberto Sassu 2022-11-21 13:39 ` Roberto Sassu 2022-11-22 19:39 ` Mimi Zohar 2022-11-23 12:56 ` Roberto Sassu 2022-11-23 13:40 ` Mimi Zohar 2022-11-23 13:49 ` Roberto Sassu 2022-11-30 14:41 ` Roberto Sassu 2022-11-30 16:22 ` Mimi Zohar 2022-11-30 16:24 ` Roberto Sassu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).