All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eric Biggers <ebiggers3@gmail.com>
To: Andy Lutomirski <luto@amacapital.net>
Cc: linux-crypto@vger.kernel.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"kernel-hardening@lists.openwall.com"
	<kernel-hardening@lists.openwall.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Andrew Lutomirski <luto@kernel.org>,
	Stephan Mueller <smueller@chronox.de>
Subject: [kernel-hardening] Re: Remaining crypto API regressions with CONFIG_VMAP_STACK
Date: Fri, 9 Dec 2016 21:55:31 -0800	[thread overview]
Message-ID: <20161210055531.GB6846@zzz> (raw)
In-Reply-To: <CALCETrW=+3u3P8Xva+0ck9=fr-mD6azPtTkOQ3uQO+GoOA6FcQ@mail.gmail.com>

On Fri, Dec 09, 2016 at 09:25:38PM -0800, Andy Lutomirski wrote:
> > The following crypto drivers initialize a scatterlist to point into an
> > ahash_request, which may have been allocated on the stack with
> > AHASH_REQUEST_ON_STACK():
> >
> >         drivers/crypto/bfin_crc.c:351
> >         drivers/crypto/qce/sha.c:299
> >         drivers/crypto/sahara.c:973,988
> >         drivers/crypto/talitos.c:1910
> 
> This are impossible or highly unlikely on x86.
> 
> >         drivers/crypto/ccp/ccp-crypto-aes-cmac.c:105,119,142
> >         drivers/crypto/ccp/ccp-crypto-sha.c:95,109,124
> 
> These
> 
> >         drivers/crypto/qce/sha.c:325
> 
> This is impossible on x86.
> 

Thanks for looking into these.  I didn't investigate who/what is likely to be
using each driver.

Of course I would not be surprised to see people want to start supporting
virtually mapped stacks on other architectures too.

> >
> > The "good" news with these bugs is that on x86_64 without CONFIG_DEBUG_SG=y or
> > CONFIG_DEBUG_VIRTUAL=y, you can still do virt_to_page() and then page_address()
> > on a vmalloc address and get back the same address, even though you aren't
> > *supposed* to be able to do this.  This will make things still work for most
> > people.  The bad news is that if you happen to have consumed just about 1 page
> > (or N pages) of your stack at the time you call the crypto API, your stack
> > buffer may actually span physically non-contiguous pages, so the crypto
> > algorithm will scribble over some unrelated page.
> 
> Are you sure?  If it round-trips to the same virtual address, it
> doesn't matter if the buffer is contiguous.

You may be right, I didn't test this.  The hash_walk and blkcipher_walk code do
go page by page, but I suppose on x86_64 it would just step from one bogus
"struct page" to the adjacent one and still map it to the original virtual
address.

Eric

WARNING: multiple messages have this Message-ID (diff)
From: Eric Biggers <ebiggers3@gmail.com>
To: Andy Lutomirski <luto@amacapital.net>
Cc: linux-crypto@vger.kernel.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"kernel-hardening@lists.openwall.com"
	<kernel-hardening@lists.openwall.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Andrew Lutomirski <luto@kernel.org>,
	Stephan Mueller <smueller@chronox.de>
Subject: Re: Remaining crypto API regressions with CONFIG_VMAP_STACK
Date: Fri, 9 Dec 2016 21:55:31 -0800	[thread overview]
Message-ID: <20161210055531.GB6846@zzz> (raw)
In-Reply-To: <CALCETrW=+3u3P8Xva+0ck9=fr-mD6azPtTkOQ3uQO+GoOA6FcQ@mail.gmail.com>

On Fri, Dec 09, 2016 at 09:25:38PM -0800, Andy Lutomirski wrote:
> > The following crypto drivers initialize a scatterlist to point into an
> > ahash_request, which may have been allocated on the stack with
> > AHASH_REQUEST_ON_STACK():
> >
> >         drivers/crypto/bfin_crc.c:351
> >         drivers/crypto/qce/sha.c:299
> >         drivers/crypto/sahara.c:973,988
> >         drivers/crypto/talitos.c:1910
> 
> This are impossible or highly unlikely on x86.
> 
> >         drivers/crypto/ccp/ccp-crypto-aes-cmac.c:105,119,142
> >         drivers/crypto/ccp/ccp-crypto-sha.c:95,109,124
> 
> These
> 
> >         drivers/crypto/qce/sha.c:325
> 
> This is impossible on x86.
> 

Thanks for looking into these.  I didn't investigate who/what is likely to be
using each driver.

Of course I would not be surprised to see people want to start supporting
virtually mapped stacks on other architectures too.

> >
> > The "good" news with these bugs is that on x86_64 without CONFIG_DEBUG_SG=y or
> > CONFIG_DEBUG_VIRTUAL=y, you can still do virt_to_page() and then page_address()
> > on a vmalloc address and get back the same address, even though you aren't
> > *supposed* to be able to do this.  This will make things still work for most
> > people.  The bad news is that if you happen to have consumed just about 1 page
> > (or N pages) of your stack at the time you call the crypto API, your stack
> > buffer may actually span physically non-contiguous pages, so the crypto
> > algorithm will scribble over some unrelated page.
> 
> Are you sure?  If it round-trips to the same virtual address, it
> doesn't matter if the buffer is contiguous.

You may be right, I didn't test this.  The hash_walk and blkcipher_walk code do
go page by page, but I suppose on x86_64 it would just step from one bogus
"struct page" to the adjacent one and still map it to the original virtual
address.

Eric

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Eric Biggers <ebiggers3@gmail.com>
To: Andy Lutomirski <luto@amacapital.net>
Cc: linux-crypto@vger.kernel.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"kernel-hardening@lists.openwall.com" 
	<kernel-hardening@lists.openwall.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Andrew Lutomirski <luto@kernel.org>,
	Stephan Mueller <smueller@chronox.de>
Subject: Re: Remaining crypto API regressions with CONFIG_VMAP_STACK
Date: Fri, 9 Dec 2016 21:55:31 -0800	[thread overview]
Message-ID: <20161210055531.GB6846@zzz> (raw)
In-Reply-To: <CALCETrW=+3u3P8Xva+0ck9=fr-mD6azPtTkOQ3uQO+GoOA6FcQ@mail.gmail.com>

On Fri, Dec 09, 2016 at 09:25:38PM -0800, Andy Lutomirski wrote:
> > The following crypto drivers initialize a scatterlist to point into an
> > ahash_request, which may have been allocated on the stack with
> > AHASH_REQUEST_ON_STACK():
> >
> >         drivers/crypto/bfin_crc.c:351
> >         drivers/crypto/qce/sha.c:299
> >         drivers/crypto/sahara.c:973,988
> >         drivers/crypto/talitos.c:1910
> 
> This are impossible or highly unlikely on x86.
> 
> >         drivers/crypto/ccp/ccp-crypto-aes-cmac.c:105,119,142
> >         drivers/crypto/ccp/ccp-crypto-sha.c:95,109,124
> 
> These
> 
> >         drivers/crypto/qce/sha.c:325
> 
> This is impossible on x86.
> 

Thanks for looking into these.  I didn't investigate who/what is likely to be
using each driver.

Of course I would not be surprised to see people want to start supporting
virtually mapped stacks on other architectures too.

> >
> > The "good" news with these bugs is that on x86_64 without CONFIG_DEBUG_SG=y or
> > CONFIG_DEBUG_VIRTUAL=y, you can still do virt_to_page() and then page_address()
> > on a vmalloc address and get back the same address, even though you aren't
> > *supposed* to be able to do this.  This will make things still work for most
> > people.  The bad news is that if you happen to have consumed just about 1 page
> > (or N pages) of your stack at the time you call the crypto API, your stack
> > buffer may actually span physically non-contiguous pages, so the crypto
> > algorithm will scribble over some unrelated page.
> 
> Are you sure?  If it round-trips to the same virtual address, it
> doesn't matter if the buffer is contiguous.

You may be right, I didn't test this.  The hash_walk and blkcipher_walk code do
go page by page, but I suppose on x86_64 it would just step from one bogus
"struct page" to the adjacent one and still map it to the original virtual
address.

Eric

  parent reply	other threads:[~2016-12-10  5:55 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-09 23:08 [kernel-hardening] Remaining crypto API regressions with CONFIG_VMAP_STACK Eric Biggers
2016-12-09 23:08 ` Eric Biggers
2016-12-09 23:08 ` Eric Biggers
2016-12-10  5:25 ` [kernel-hardening] " Andy Lutomirski
2016-12-10  5:25   ` Andy Lutomirski
2016-12-10  5:25   ` Andy Lutomirski
2016-12-10  5:32   ` [kernel-hardening] " Herbert Xu
2016-12-10  5:32     ` Herbert Xu
2016-12-10  5:32     ` Herbert Xu
2016-12-10  6:03     ` [kernel-hardening] " Eric Biggers
2016-12-10  6:03       ` Eric Biggers
2016-12-10  8:16       ` Herbert Xu
2016-12-10  8:16         ` Herbert Xu
2016-12-10  8:39         ` Eric Biggers
2016-12-10  8:39           ` Eric Biggers
2016-12-10  8:39           ` Eric Biggers
2016-12-10  5:37   ` [kernel-hardening] " Herbert Xu
2016-12-10  5:37     ` Herbert Xu
2016-12-10  5:37     ` Herbert Xu
2016-12-10  6:30     ` [kernel-hardening] " Eric Biggers
2016-12-10  6:30       ` Eric Biggers
2016-12-10 14:45     ` Jason A. Donenfeld
2016-12-10 14:45       ` Jason A. Donenfeld
2016-12-10 17:48       ` Andy Lutomirski
2016-12-10 17:48         ` Andy Lutomirski
2016-12-10  5:55   ` Eric Biggers [this message]
2016-12-10  5:55     ` Eric Biggers
2016-12-10  5:55     ` Eric Biggers
2016-12-11 19:13 ` [kernel-hardening] " Andy Lutomirski
2016-12-11 19:13   ` Andy Lutomirski
2016-12-11 19:13   ` Andy Lutomirski
2016-12-11 23:31   ` [kernel-hardening] " Eric Biggers
2016-12-11 23:31     ` Eric Biggers
2016-12-11 23:31     ` Eric Biggers
2016-12-12 18:34 ` [kernel-hardening] " Andy Lutomirski
2016-12-12 18:34   ` Andy Lutomirski
2016-12-12 18:34   ` Andy Lutomirski
2016-12-12 18:45   ` [kernel-hardening] " Gary R Hook
2016-12-12 18:45     ` Gary R Hook
2016-12-12 18:45     ` Gary R Hook
2016-12-12 18:45     ` Gary R Hook
2016-12-13  3:39     ` [kernel-hardening] " Herbert Xu
2016-12-13  3:39       ` Herbert Xu
2016-12-13  3:39       ` Herbert Xu
2016-12-13  3:39   ` [kernel-hardening] " Herbert Xu
2016-12-13  3:39     ` Herbert Xu
2016-12-13  3:39     ` Herbert Xu
2016-12-13 17:06     ` [kernel-hardening] " Andy Lutomirski
2016-12-13 17:06       ` Andy Lutomirski
2016-12-13 17:06       ` Andy Lutomirski
2016-12-14  4:56       ` [kernel-hardening] " Herbert Xu
2016-12-14  4:56         ` Herbert Xu
2016-12-14  4:56         ` Herbert Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161210055531.GB6846@zzz \
    --to=ebiggers3@gmail.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=luto@amacapital.net \
    --cc=luto@kernel.org \
    --cc=smueller@chronox.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.