diff mbox series

ima: Make a copy of sig and digest in asymmetric_verify()

Message ID 20221104122023.1750333-1-roberto.sassu@huaweicloud.com
State New
Headers
Series ima: Make a copy of sig and digest in asymmetric_verify() |

Commit Message

Roberto Sassu Nov. 4, 2022, 12:20 p.m. UTC 
  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(-)

Comments

Roberto Sassu Nov. 21, 2022, 1:39 p.m. UTC | #1 
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);
Mimi Zohar Nov. 22, 2022, 7:39 p.m. UTC | #2 
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.
Roberto Sassu Nov. 23, 2022, 12:56 p.m. UTC | #3 
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
Mimi Zohar Nov. 23, 2022, 1:40 p.m. UTC | #4 
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.
Roberto Sassu Nov. 23, 2022, 1:49 p.m. UTC | #5 
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
Roberto Sassu Nov. 30, 2022, 2:41 p.m. UTC | #6 
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
Mimi Zohar Nov. 30, 2022, 4:22 p.m. UTC | #7 
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?
Roberto Sassu Nov. 30, 2022, 4:24 p.m. UTC | #8 
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 mbox series

Patch

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);