[PATCH v2 1/2] evm: Alloc evm_digest in evm_verify_hmac() if CONFIG_VMAP_STACK=y

[PATCH v2 1/2] evm: Alloc evm_digest in evm_verify_hmac() if CONFIG_VMAP_STACK=y

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
adjacent pages.

Fix this by checking if CONFIG_VMAP_STACK is enabled. If yes, allocate an
evm_digest structure, and use that instead of the in-stack counterpart.

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/evm/evm_main.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 23d484e05e6f..7f76d6103f2e 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -174,6 +174,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 	struct signature_v2_hdr *hdr;
 	enum integrity_status evm_status = INTEGRITY_PASS;
 	struct evm_digest digest;
+	struct evm_digest *digest_ptr = &digest;
 	struct inode *inode;
 	int rc, xattr_len, evm_immutable = 0;
 
@@ -231,14 +232,26 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		}
 
 		hdr = (struct signature_v2_hdr *)xattr_data;
-		digest.hdr.algo = hdr->hash_algo;
+
+		if (IS_ENABLED(CONFIG_VMAP_STACK)) {
+			digest_ptr = kmalloc(sizeof(*digest_ptr), GFP_NOFS);
+			if (!digest_ptr) {
+				rc = -ENOMEM;
+				break;
+			}
+		}
+
+		digest_ptr->hdr.algo = hdr->hash_algo;
+
 		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
-				   xattr_value_len, xattr_data->type, &digest);
+				   xattr_value_len, xattr_data->type,
+				   digest_ptr);
 		if (rc)
 			break;
 		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
 					(const char *)xattr_data, xattr_len,
-					digest.digest, digest.hdr.length);
+					digest_ptr->digest,
+					digest_ptr->hdr.length);
 		if (!rc) {
 			inode = d_backing_inode(dentry);
 
@@ -268,8 +281,11 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		else
 			evm_status = INTEGRITY_FAIL;
 	}
-	pr_debug("digest: (%d) [%*phN]\n", digest.hdr.length, digest.hdr.length,
-		  digest.digest);
+	pr_debug("digest: (%d) [%*phN]\n", digest_ptr->hdr.length,
+		 digest_ptr->hdr.length, digest_ptr->digest);
+
+	if (digest_ptr && digest_ptr != &digest)
+		kfree(digest_ptr);
 out:
 	if (iint)
 		iint->evm_status = evm_status;
-- 
2.25.1

Re: [PATCH v2 1/2] evm: Alloc evm_digest in evm_verify_hmac() if CONFIG_VMAP_STACK=y

[Eric Biggers]

On Thu, Dec 01, 2022 at 11:06:24AM +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> adjacent pages.
> 
> Fix this by checking if CONFIG_VMAP_STACK is enabled. If yes, allocate an
> evm_digest structure, and use that instead of the in-stack counterpart.
> 
> 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/evm/evm_main.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 23d484e05e6f..7f76d6103f2e 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -174,6 +174,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  	struct signature_v2_hdr *hdr;
>  	enum integrity_status evm_status = INTEGRITY_PASS;
>  	struct evm_digest digest;
> +	struct evm_digest *digest_ptr = &digest;
>  	struct inode *inode;
>  	int rc, xattr_len, evm_immutable = 0;
>  
> @@ -231,14 +232,26 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  		}
>  
>  		hdr = (struct signature_v2_hdr *)xattr_data;
> -		digest.hdr.algo = hdr->hash_algo;
> +
> +		if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> +			digest_ptr = kmalloc(sizeof(*digest_ptr), GFP_NOFS);
> +			if (!digest_ptr) {
> +				rc = -ENOMEM;
> +				break;
> +			}
> +		}
> +
> +		digest_ptr->hdr.algo = hdr->hash_algo;
> +
>  		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> -				   xattr_value_len, xattr_data->type, &digest);
> +				   xattr_value_len, xattr_data->type,
> +				   digest_ptr);
>  		if (rc)
>  			break;
>  		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
>  					(const char *)xattr_data, xattr_len,
> -					digest.digest, digest.hdr.length);
> +					digest_ptr->digest,
> +					digest_ptr->hdr.length);
>  		if (!rc) {
>  			inode = d_backing_inode(dentry);
>  
> @@ -268,8 +281,11 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  		else
>  			evm_status = INTEGRITY_FAIL;
>  	}
> -	pr_debug("digest: (%d) [%*phN]\n", digest.hdr.length, digest.hdr.length,
> -		  digest.digest);
> +	pr_debug("digest: (%d) [%*phN]\n", digest_ptr->hdr.length,
> +		 digest_ptr->hdr.length, digest_ptr->digest);
> +
> +	if (digest_ptr && digest_ptr != &digest)
> +		kfree(digest_ptr);

What is the actual problem here?  Where is a scatterlist being created from this
buffer?  AFAICS it never happens.

- Eric

Re: [PATCH v2 1/2] evm: Alloc evm_digest in evm_verify_hmac() if CONFIG_VMAP_STACK=y

[Mimi Zohar]

On Thu, 2022-12-01 at 10:53 -0800, Eric Biggers wrote:
> On Thu, Dec 01, 2022 at 11:06:24AM +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > adjacent pages.
> > 
> > Fix this by checking if CONFIG_VMAP_STACK is enabled. If yes, allocate an
> > evm_digest structure, and use that instead of the in-stack cbounterpart.
> > 
> > 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/evm/evm_main.c | 26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > index 23d484e05e6f..7f76d6103f2e 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -174,6 +174,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> >  	struct signature_v2_hdr *hdr;
> >  	enum integrity_status evm_status = INTEGRITY_PASS;
> >  	struct evm_digest digest;
> > +	struct evm_digest *digest_ptr = &digest;
> >  	struct inode *inode;
> >  	int rc, xattr_len, evm_immutable = 0;
> >  
> > @@ -231,14 +232,26 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> >  		}
> >  
> >  		hdr = (struct signature_v2_hdr *)xattr_data;
> > -		digest.hdr.algo = hdr->hash_algo;
> > +
> > +		if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> > +			digest_ptr = kmalloc(sizeof(*digest_ptr), GFP_NOFS);
> > +			if (!digest_ptr) {
> > +				rc = -ENOMEM;
> > +				break;
> > +			}
> > +		}
> > +
> > +		digest_ptr->hdr.algo = hdr->hash_algo;
> > +
> >  		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> > -				   xattr_value_len, xattr_data->type, &digest);
> > +				   xattr_value_len, xattr_data->type,
> > +				   digest_ptr);
> >  		if (rc)
> >  			break;
> >  		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> >  					(const char *)xattr_data, xattr_len,
> > -					digest.digest, digest.hdr.length);
> > +					digest_ptr->digest,
> > +					digest_ptr->hdr.length);
> >  		if (!rc) {
> >  			inode = d_backing_inode(dentry);
> >  
> > @@ -268,8 +281,11 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> >  		else
> >  			evm_status = INTEGRITY_FAIL;
> >  	}
> > -	pr_debug("digest: (%d) [%*phN]\n", digest.hdr.length, digest.hdr.length,
> > -		  digest.digest);
> > +	pr_debug("digest: (%d) [%*phN]\n", digest_ptr->hdr.length,
> > +		 digest_ptr->hdr.length, digest_ptr->digest);
> > +
> > +	if (digest_ptr && digest_ptr != &digest)
> > +		kfree(digest_ptr);
> 
> What is the actual problem here?  Where is a scatterlist being created from this
> buffer?  AFAICS it never happens.

Enabling CONFIG_VMAP_STACK is the culprit, which triggers the BUG_ON
only when CONFIG_DEBUG_SG is enabled as well.

Refer to commit ba14a194a434 ("fork: Add generic vmalloced stack
support").

-- 
thanks,

Mimi

Re: [PATCH v2 1/2] evm: Alloc evm_digest in evm_verify_hmac() if CONFIG_VMAP_STACK=y

[Eric Biggers]

On Thu, Dec 01, 2022 at 02:08:58PM -0500, Mimi Zohar wrote:
> On Thu, 2022-12-01 at 10:53 -0800, Eric Biggers wrote:
> > On Thu, Dec 01, 2022 at 11:06:24AM +0100, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > adjacent pages.
> > > 
> > > Fix this by checking if CONFIG_VMAP_STACK is enabled. If yes, allocate an
> > > evm_digest structure, and use that instead of the in-stack cbounterpart.
> > > 
> > > 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/evm/evm_main.c | 26 +++++++++++++++++++++-----
> > >  1 file changed, 21 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > > index 23d484e05e6f..7f76d6103f2e 100644
> > > --- a/security/integrity/evm/evm_main.c
> > > +++ b/security/integrity/evm/evm_main.c
> > > @@ -174,6 +174,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > >  	struct signature_v2_hdr *hdr;
> > >  	enum integrity_status evm_status = INTEGRITY_PASS;
> > >  	struct evm_digest digest;
> > > +	struct evm_digest *digest_ptr = &digest;
> > >  	struct inode *inode;
> > >  	int rc, xattr_len, evm_immutable = 0;
> > >  
> > > @@ -231,14 +232,26 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > >  		}
> > >  
> > >  		hdr = (struct signature_v2_hdr *)xattr_data;
> > > -		digest.hdr.algo = hdr->hash_algo;
> > > +
> > > +		if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> > > +			digest_ptr = kmalloc(sizeof(*digest_ptr), GFP_NOFS);
> > > +			if (!digest_ptr) {
> > > +				rc = -ENOMEM;
> > > +				break;
> > > +			}
> > > +		}
> > > +
> > > +		digest_ptr->hdr.algo = hdr->hash_algo;
> > > +
> > >  		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> > > -				   xattr_value_len, xattr_data->type, &digest);
> > > +				   xattr_value_len, xattr_data->type,
> > > +				   digest_ptr);
> > >  		if (rc)
> > >  			break;
> > >  		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> > >  					(const char *)xattr_data, xattr_len,
> > > -					digest.digest, digest.hdr.length);
> > > +					digest_ptr->digest,
> > > +					digest_ptr->hdr.length);
> > >  		if (!rc) {
> > >  			inode = d_backing_inode(dentry);
> > >  
> > > @@ -268,8 +281,11 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > >  		else
> > >  			evm_status = INTEGRITY_FAIL;
> > >  	}
> > > -	pr_debug("digest: (%d) [%*phN]\n", digest.hdr.length, digest.hdr.length,
> > > -		  digest.digest);
> > > +	pr_debug("digest: (%d) [%*phN]\n", digest_ptr->hdr.length,
> > > +		 digest_ptr->hdr.length, digest_ptr->digest);
> > > +
> > > +	if (digest_ptr && digest_ptr != &digest)
> > > +		kfree(digest_ptr);
> > 
> > What is the actual problem here?  Where is a scatterlist being created from this
> > buffer?  AFAICS it never happens.
> 
> Enabling CONFIG_VMAP_STACK is the culprit, which triggers the BUG_ON
> only when CONFIG_DEBUG_SG is enabled as well.
> 
> Refer to commit ba14a194a434 ("fork: Add generic vmalloced stack
> support").

I'm asking about where the actual bug is.  Where is a scatterlist being created
to represent an on-disk buffer...

- Eric

Re: [PATCH v2 1/2] evm: Alloc evm_digest in evm_verify_hmac() if CONFIG_VMAP_STACK=y

[Roberto Sassu]

On Thu, 2022-12-01 at 10:53 -0800, Eric Biggers wrote:
> On Thu, Dec 01, 2022 at 11:06:24AM +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > adjacent pages.
> > 
> > Fix this by checking if CONFIG_VMAP_STACK is enabled. If yes, allocate an
> > evm_digest structure, and use that instead of the in-stack counterpart.
> > 
> > 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/evm/evm_main.c | 26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > index 23d484e05e6f..7f76d6103f2e 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -174,6 +174,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> >  	struct signature_v2_hdr *hdr;
> >  	enum integrity_status evm_status = INTEGRITY_PASS;
> >  	struct evm_digest digest;
> > +	struct evm_digest *digest_ptr = &digest;
> >  	struct inode *inode;
> >  	int rc, xattr_len, evm_immutable = 0;
> >  
> > @@ -231,14 +232,26 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> >  		}
> >  
> >  		hdr = (struct signature_v2_hdr *)xattr_data;
> > -		digest.hdr.algo = hdr->hash_algo;
> > +
> > +		if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> > +			digest_ptr = kmalloc(sizeof(*digest_ptr), GFP_NOFS);
> > +			if (!digest_ptr) {
> > +				rc = -ENOMEM;
> > +				break;
> > +			}
> > +		}
> > +
> > +		digest_ptr->hdr.algo = hdr->hash_algo;
> > +
> >  		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> > -				   xattr_value_len, xattr_data->type, &digest);
> > +				   xattr_value_len, xattr_data->type,
> > +				   digest_ptr);
> >  		if (rc)
> >  			break;
> >  		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> >  					(const char *)xattr_data, xattr_len,
> > -					digest.digest, digest.hdr.length);
> > +					digest_ptr->digest,
> > +					digest_ptr->hdr.length);
> >  		if (!rc) {
> >  			inode = d_backing_inode(dentry);
> >  
> > @@ -268,8 +281,11 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> >  		else
> >  			evm_status = INTEGRITY_FAIL;
> >  	}
> > -	pr_debug("digest: (%d) [%*phN]\n", digest.hdr.length, digest.hdr.length,
> > -		  digest.digest);
> > +	pr_debug("digest: (%d) [%*phN]\n", digest_ptr->hdr.length,
> > +		 digest_ptr->hdr.length, digest_ptr->digest);
> > +
> > +	if (digest_ptr && digest_ptr != &digest)
> > +		kfree(digest_ptr);
> 
> What is the actual problem here?  Where is a scatterlist being created from this
> buffer?  AFAICS it never happens.

Hi Eric

it is in public_key_verify_signature(), called by asymmetric_verify()
and integrity_digsig_verify().

Roberto

Re: [PATCH v2 1/2] evm: Alloc evm_digest in evm_verify_hmac() if CONFIG_VMAP_STACK=y

[Eric Biggers]

On Fri, Dec 02, 2022 at 08:58:21AM +0100, Roberto Sassu wrote:
> On Thu, 2022-12-01 at 10:53 -0800, Eric Biggers wrote:
> > On Thu, Dec 01, 2022 at 11:06:24AM +0100, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > adjacent pages.
> > > 
> > > Fix this by checking if CONFIG_VMAP_STACK is enabled. If yes, allocate an
> > > evm_digest structure, and use that instead of the in-stack counterpart.
> > > 
> > > 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/evm/evm_main.c | 26 +++++++++++++++++++++-----
> > >  1 file changed, 21 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > > index 23d484e05e6f..7f76d6103f2e 100644
> > > --- a/security/integrity/evm/evm_main.c
> > > +++ b/security/integrity/evm/evm_main.c
> > > @@ -174,6 +174,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > >  	struct signature_v2_hdr *hdr;
> > >  	enum integrity_status evm_status = INTEGRITY_PASS;
> > >  	struct evm_digest digest;
> > > +	struct evm_digest *digest_ptr = &digest;
> > >  	struct inode *inode;
> > >  	int rc, xattr_len, evm_immutable = 0;
> > >  
> > > @@ -231,14 +232,26 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > >  		}
> > >  
> > >  		hdr = (struct signature_v2_hdr *)xattr_data;
> > > -		digest.hdr.algo = hdr->hash_algo;
> > > +
> > > +		if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> > > +			digest_ptr = kmalloc(sizeof(*digest_ptr), GFP_NOFS);
> > > +			if (!digest_ptr) {
> > > +				rc = -ENOMEM;
> > > +				break;
> > > +			}
> > > +		}
> > > +
> > > +		digest_ptr->hdr.algo = hdr->hash_algo;
> > > +
> > >  		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> > > -				   xattr_value_len, xattr_data->type, &digest);
> > > +				   xattr_value_len, xattr_data->type,
> > > +				   digest_ptr);
> > >  		if (rc)
> > >  			break;
> > >  		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> > >  					(const char *)xattr_data, xattr_len,
> > > -					digest.digest, digest.hdr.length);
> > > +					digest_ptr->digest,
> > > +					digest_ptr->hdr.length);
> > >  		if (!rc) {
> > >  			inode = d_backing_inode(dentry);
> > >  
> > > @@ -268,8 +281,11 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > >  		else
> > >  			evm_status = INTEGRITY_FAIL;
> > >  	}
> > > -	pr_debug("digest: (%d) [%*phN]\n", digest.hdr.length, digest.hdr.length,
> > > -		  digest.digest);
> > > +	pr_debug("digest: (%d) [%*phN]\n", digest_ptr->hdr.length,
> > > +		 digest_ptr->hdr.length, digest_ptr->digest);
> > > +
> > > +	if (digest_ptr && digest_ptr != &digest)
> > > +		kfree(digest_ptr);
> > 
> > What is the actual problem here?  Where is a scatterlist being created from this
> > buffer?  AFAICS it never happens.
> 
> Hi Eric
> 
> it is in public_key_verify_signature(), called by asymmetric_verify()
> and integrity_digsig_verify().
> 

Hmm, that's several steps down the stack then.  And not something I had
expected.

Perhaps this should be fixed in public_key_verify_signature() instead?  It
already does a kmalloc(), so that allocation size just could be made a bit
larger to get space for a temporary copy of 's' and 'digest'.

Or at the very least, struct public_key_signature should have a *very* clear
comment saying that the 's' and 'digest' fields must be located in physically
contiguous memory...

- Eric

Re: [PATCH v2 1/2] evm: Alloc evm_digest in evm_verify_hmac() if CONFIG_VMAP_STACK=y

[Roberto Sassu]

On Fri, 2022-12-02 at 10:49 -0800, Eric Biggers wrote:
> On Fri, Dec 02, 2022 at 08:58:21AM +0100, Roberto Sassu wrote:
> > On Thu, 2022-12-01 at 10:53 -0800, Eric Biggers wrote:
> > > On Thu, Dec 01, 2022 at 11:06:24AM +0100, Roberto Sassu wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > 
> > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > > adjacent pages.
> > > > 
> > > > Fix this by checking if CONFIG_VMAP_STACK is enabled. If yes, allocate an
> > > > evm_digest structure, and use that instead of the in-stack counterpart.
> > > > 
> > > > 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/evm/evm_main.c | 26 +++++++++++++++++++++-----
> > > >  1 file changed, 21 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > > > index 23d484e05e6f..7f76d6103f2e 100644
> > > > --- a/security/integrity/evm/evm_main.c
> > > > +++ b/security/integrity/evm/evm_main.c
> > > > @@ -174,6 +174,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > > >  	struct signature_v2_hdr *hdr;
> > > >  	enum integrity_status evm_status = INTEGRITY_PASS;
> > > >  	struct evm_digest digest;
> > > > +	struct evm_digest *digest_ptr = &digest;
> > > >  	struct inode *inode;
> > > >  	int rc, xattr_len, evm_immutable = 0;
> > > >  
> > > > @@ -231,14 +232,26 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > > >  		}
> > > >  
> > > >  		hdr = (struct signature_v2_hdr *)xattr_data;
> > > > -		digest.hdr.algo = hdr->hash_algo;
> > > > +
> > > > +		if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> > > > +			digest_ptr = kmalloc(sizeof(*digest_ptr), GFP_NOFS);
> > > > +			if (!digest_ptr) {
> > > > +				rc = -ENOMEM;
> > > > +				break;
> > > > +			}
> > > > +		}
> > > > +
> > > > +		digest_ptr->hdr.algo = hdr->hash_algo;
> > > > +
> > > >  		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> > > > -				   xattr_value_len, xattr_data->type, &digest);
> > > > +				   xattr_value_len, xattr_data->type,
> > > > +				   digest_ptr);
> > > >  		if (rc)
> > > >  			break;
> > > >  		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> > > >  					(const char *)xattr_data, xattr_len,
> > > > -					digest.digest, digest.hdr.length);
> > > > +					digest_ptr->digest,
> > > > +					digest_ptr->hdr.length);
> > > >  		if (!rc) {
> > > >  			inode = d_backing_inode(dentry);
> > > >  
> > > > @@ -268,8 +281,11 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > > >  		else
> > > >  			evm_status = INTEGRITY_FAIL;
> > > >  	}
> > > > -	pr_debug("digest: (%d) [%*phN]\n", digest.hdr.length, digest.hdr.length,
> > > > -		  digest.digest);
> > > > +	pr_debug("digest: (%d) [%*phN]\n", digest_ptr->hdr.length,
> > > > +		 digest_ptr->hdr.length, digest_ptr->digest);
> > > > +
> > > > +	if (digest_ptr && digest_ptr != &digest)
> > > > +		kfree(digest_ptr);
> > > 
> > > What is the actual problem here?  Where is a scatterlist being created from this
> > > buffer?  AFAICS it never happens.
> > 
> > Hi Eric
> > 
> > it is in public_key_verify_signature(), called by asymmetric_verify()
> > and integrity_digsig_verify().
> > 
> 
> Hmm, that's several steps down the stack then.  And not something I had
> expected.
> 
> Perhaps this should be fixed in public_key_verify_signature() instead?  It
> already does a kmalloc(), so that allocation size just could be made a bit
> larger to get space for a temporary copy of 's' and 'digest'.

Mimi asked to fix it in both IMA and EVM.

> Or at the very least, struct public_key_signature should have a *very* clear
> comment saying that the 's' and 'digest' fields must be located in physically
> contiguous memory...

That I could add as an additional patch.

Thanks

Roberto

Re: [PATCH v2 1/2] evm: Alloc evm_digest in evm_verify_hmac() if CONFIG_VMAP_STACK=y

[Mimi Zohar]

On Mon, 2022-12-05 at 09:22 +0100, Roberto Sassu wrote:
> On Fri, 2022-12-02 at 10:49 -0800, Eric Biggers wrote:
> > On Fri, Dec 02, 2022 at 08:58:21AM +0100, Roberto Sassu wrote:
> > > On Thu, 2022-12-01 at 10:53 -0800, Eric Biggers wrote:
> > > > On Thu, Dec 01, 2022 at 11:06:24AM +0100, Roberto Sassu wrote:
> > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > 
> > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > > > adjacent pages.
> > > > > 
> > > > > Fix this by checking if CONFIG_VMAP_STACK is enabled. If yes, allocate an
> > > > > evm_digest structure, and use that instead of the in-stack counterpart.
> > > > > 
> > > > > 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/evm/evm_main.c | 26 +++++++++++++++++++++-----
> > > > >  1 file changed, 21 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > > > > index 23d484e05e6f..7f76d6103f2e 100644
> > > > > --- a/security/integrity/evm/evm_main.c
> > > > > +++ b/security/integrity/evm/evm_main.c
> > > > > @@ -174,6 +174,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > > > >  	struct signature_v2_hdr *hdr;
> > > > >  	enum integrity_status evm_status = INTEGRITY_PASS;
> > > > >  	struct evm_digest digest;
> > > > > +	struct evm_digest *digest_ptr = &digest;
> > > > >  	struct inode *inode;
> > > > >  	int rc, xattr_len, evm_immutable = 0;
> > > > >  
> > > > > @@ -231,14 +232,26 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > > > >  		}
> > > > >  
> > > > >  		hdr = (struct signature_v2_hdr *)xattr_data;
> > > > > -		digest.hdr.algo = hdr->hash_algo;
> > > > > +
> > > > > +		if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> > > > > +			digest_ptr = kmalloc(sizeof(*digest_ptr), GFP_NOFS);
> > > > > +			if (!digest_ptr) {
> > > > > +				rc = -ENOMEM;
> > > > > +				break;
> > > > > +			}
> > > > > +		}
> > > > > +
> > > > > +		digest_ptr->hdr.algo = hdr->hash_algo;
> > > > > +
> > > > >  		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> > > > > -				   xattr_value_len, xattr_data->type, &digest);
> > > > > +				   xattr_value_len, xattr_data->type,
> > > > > +				   digest_ptr);
> > > > >  		if (rc)
> > > > >  			break;
> > > > >  		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> > > > >  					(const char *)xattr_data, xattr_len,
> > > > > -					digest.digest, digest.hdr.length);
> > > > > +					digest_ptr->digest,
> > > > > +					digest_ptr->hdr.length);
> > > > >  		if (!rc) {
> > > > >  			inode = d_backing_inode(dentry);
> > > > >  
> > > > > @@ -268,8 +281,11 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > > > >  		else
> > > > >  			evm_status = INTEGRITY_FAIL;
> > > > >  	}
> > > > > -	pr_debug("digest: (%d) [%*phN]\n", digest.hdr.length, digest.hdr.length,
> > > > > -		  digest.digest);
> > > > > +	pr_debug("digest: (%d) [%*phN]\n", digest_ptr->hdr.length,
> > > > > +		 digest_ptr->hdr.length, digest_ptr->digest);
> > > > > +
> > > > > +	if (digest_ptr && digest_ptr != &digest)
> > > > > +		kfree(digest_ptr);
> > > > 
> > > > What is the actual problem here?  Where is a scatterlist being created from this
> > > > buffer?  AFAICS it never happens.
> > > 
> > > Hi Eric
> > > 
> > > it is in public_key_verify_signature(), called by asymmetric_verify()
> > > and integrity_digsig_verify().
> > > 
> > 
> > Hmm, that's several steps down the stack then.  And not something I had
> > expected.
> > 
> > Perhaps this should be fixed in public_key_verify_signature() instead?  It
> > already does a kmalloc(), so that allocation size just could be made a bit
> > larger to get space for a temporary copy of 's' and 'digest'.
> 
> Mimi asked to fix it in both IMA and EVM.

At the time I thought the problem was limited to
integrity_digsig_verify() and just to the digest.

I'll leave it up to you and Eric to decide what is the preferable
solution.

> 
> > Or at the very least, struct public_key_signature should have a *very* clear
> > comment saying that the 's' and 'digest' fields must be located in physically
> > contiguous memory...
> 
> That I could add as an additional patch.

Thanks, the new patch containing the comment looks fine.

-- 
thanks,

Mimi

Re: [PATCH v2 1/2] evm: Alloc evm_digest in evm_verify_hmac() if CONFIG_VMAP_STACK=y

[Roberto Sassu]

On Wed, 2022-12-07 at 20:26 -0500, Mimi Zohar wrote:
> On Mon, 2022-12-05 at 09:22 +0100, Roberto Sassu wrote:
> > On Fri, 2022-12-02 at 10:49 -0800, Eric Biggers wrote:
> > > On Fri, Dec 02, 2022 at 08:58:21AM +0100, Roberto Sassu wrote:
> > > > On Thu, 2022-12-01 at 10:53 -0800, Eric Biggers wrote:
> > > > > On Thu, Dec 01, 2022 at 11:06:24AM +0100, Roberto Sassu wrote:
> > > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > 
> > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > > > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > > > > adjacent pages.
> > > > > > 
> > > > > > Fix this by checking if CONFIG_VMAP_STACK is enabled. If yes, allocate an
> > > > > > evm_digest structure, and use that instead of the in-stack counterpart.
> > > > > > 
> > > > > > 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/evm/evm_main.c | 26 +++++++++++++++++++++-----
> > > > > >  1 file changed, 21 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > > > > > index 23d484e05e6f..7f76d6103f2e 100644
> > > > > > --- a/security/integrity/evm/evm_main.c
> > > > > > +++ b/security/integrity/evm/evm_main.c
> > > > > > @@ -174,6 +174,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > > > > >  	struct signature_v2_hdr *hdr;
> > > > > >  	enum integrity_status evm_status = INTEGRITY_PASS;
> > > > > >  	struct evm_digest digest;
> > > > > > +	struct evm_digest *digest_ptr = &digest;
> > > > > >  	struct inode *inode;
> > > > > >  	int rc, xattr_len, evm_immutable = 0;
> > > > > >  
> > > > > > @@ -231,14 +232,26 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > > > > >  		}
> > > > > >  
> > > > > >  		hdr = (struct signature_v2_hdr *)xattr_data;
> > > > > > -		digest.hdr.algo = hdr->hash_algo;
> > > > > > +
> > > > > > +		if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> > > > > > +			digest_ptr = kmalloc(sizeof(*digest_ptr), GFP_NOFS);
> > > > > > +			if (!digest_ptr) {
> > > > > > +				rc = -ENOMEM;
> > > > > > +				break;
> > > > > > +			}
> > > > > > +		}
> > > > > > +
> > > > > > +		digest_ptr->hdr.algo = hdr->hash_algo;
> > > > > > +
> > > > > >  		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> > > > > > -				   xattr_value_len, xattr_data->type, &digest);
> > > > > > +				   xattr_value_len, xattr_data->type,
> > > > > > +				   digest_ptr);
> > > > > >  		if (rc)
> > > > > >  			break;
> > > > > >  		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> > > > > >  					(const char *)xattr_data, xattr_len,
> > > > > > -					digest.digest, digest.hdr.length);
> > > > > > +					digest_ptr->digest,
> > > > > > +					digest_ptr->hdr.length);
> > > > > >  		if (!rc) {
> > > > > >  			inode = d_backing_inode(dentry);
> > > > > >  
> > > > > > @@ -268,8 +281,11 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > > > > >  		else
> > > > > >  			evm_status = INTEGRITY_FAIL;
> > > > > >  	}
> > > > > > -	pr_debug("digest: (%d) [%*phN]\n", digest.hdr.length, digest.hdr.length,
> > > > > > -		  digest.digest);
> > > > > > +	pr_debug("digest: (%d) [%*phN]\n", digest_ptr->hdr.length,
> > > > > > +		 digest_ptr->hdr.length, digest_ptr->digest);
> > > > > > +
> > > > > > +	if (digest_ptr && digest_ptr != &digest)
> > > > > > +		kfree(digest_ptr);
> > > > > 
> > > > > What is the actual problem here?  Where is a scatterlist being created from this
> > > > > buffer?  AFAICS it never happens.
> > > > 
> > > > Hi Eric
> > > > 
> > > > it is in public_key_verify_signature(), called by asymmetric_verify()
> > > > and integrity_digsig_verify().
> > > > 
> > > 
> > > Hmm, that's several steps down the stack then.  And not something I had
> > > expected.
> > > 
> > > Perhaps this should be fixed in public_key_verify_signature() instead?  It
> > > already does a kmalloc(), so that allocation size just could be made a bit
> > > larger to get space for a temporary copy of 's' and 'digest'.
> > 
> > Mimi asked to fix it in both IMA and EVM.
> 
> At the time I thought the problem was limited to
> integrity_digsig_verify() and just to the digest.
> 
> I'll leave it up to you and Eric to decide what is the preferable
> solution.

Ok, yes. I think Eric's suggestion of making a copy in
public_key_verify_signature() is better. Will do it.

> > > Or at the very least, struct public_key_signature should have a *very* clear
> > > comment saying that the 's' and 'digest' fields must be located in physically
> > > contiguous memory...
> > 
> > That I could add as an additional patch.
> 
> Thanks, the new patch containing the comment looks fine.

Thanks, not sure if I need to keep it with the new patch (probably
not).

Roberto

[PATCH v2 2/2] ima: Alloc ima_max_digest_data in xattr_verify() if CONFIG_VMAP_STACK=y

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Similarly to evm_verify_hmac(), which allocates an evm_digest structure to
satisfy the linear mapping requirement if CONFIG_VMAP_STACK is enabled, do
the same in xattr_verify(). Allocate an ima_max_digest_data structure and
use that instead of the in-stack counterpart.

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/ima/ima_appraise.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 3e0fbbd99534..ed8f05340fe8 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -278,6 +278,7 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 			enum integrity_status *status, const char **cause)
 {
 	struct ima_max_digest_data hash;
+	struct ima_max_digest_data *hash_ptr = &hash;
 	struct signature_v2_hdr *sig;
 	int rc = -EINVAL, hash_start = 0;
 	int mask;
@@ -376,8 +377,17 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 			break;
 		}
 
+		if (IS_ENABLED(CONFIG_VMAP_STACK)) {
+			hash_ptr = kmalloc(sizeof(*hash_ptr), GFP_KERNEL);
+			if (!hash_ptr) {
+				*cause = "out-of-memory";
+				*status = INTEGRITY_FAIL;
+				break;
+			}
+		}
+
 		rc = calc_file_id_hash(IMA_VERITY_DIGSIG, iint->ima_hash->algo,
-				       iint->ima_hash->digest, &hash.hdr);
+				       iint->ima_hash->digest, &hash_ptr->hdr);
 		if (rc) {
 			*cause = "sigv3-hashing-error";
 			*status = INTEGRITY_FAIL;
@@ -386,8 +396,8 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 
 		rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
 					     (const char *)xattr_value,
-					     xattr_len, hash.digest,
-					     hash.hdr.length);
+					     xattr_len, hash_ptr->digest,
+					     hash_ptr->hdr.length);
 		if (rc) {
 			*cause = "invalid-verity-signature";
 			*status = INTEGRITY_FAIL;
@@ -402,6 +412,9 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 		break;
 	}
 
+	if (hash_ptr && hash_ptr != &hash)
+		kfree(hash_ptr);
+
 	return rc;
 }
 
-- 
2.25.1

Re: [PATCH v2 2/2] ima: Alloc ima_max_digest_data in xattr_verify() if CONFIG_VMAP_STACK=y

[Eric Biggers]

On Thu, Dec 01, 2022 at 11:06:25AM +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Similarly to evm_verify_hmac(), which allocates an evm_digest structure to
> satisfy the linear mapping requirement if CONFIG_VMAP_STACK is enabled, do
> the same in xattr_verify(). Allocate an ima_max_digest_data structure and
> use that instead of the in-stack counterpart.
> 
> 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>

Likewise, what is the actual problem here?  Where specifically is a scatterlist
being used to represent an on-stack buffer?

- Eric