| Message ID | 20221104122023.1750333-1-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ima: Make a copy of sig and digest in asymmetric_verify() | expand |
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);
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.
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
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.
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
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
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?
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
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);