| Message ID | 1408386997-14690-1-git-send-email-gmazyland@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
On 18.08.2014 20:36, Milan Broz wrote: > The commit > 298a9fa08a1577211d42a75e8fc073baef61e0d9 > dm crypt: use per-bio data > causes OOPS on 32bit i686 architecture > > BUG: unable to handle kernel paging request at 20000000 > IP: [<e0fe2433>] clone_endio+0x13/0xe0 [dm_mod] > ... > > [<c1257b61>] bio_endio+0x61/0x90 > [<e142476c>] crypt_dec_pending+0x8c/0xd0 [dm_crypt] > [<e142666f>] kcryptd_crypt+0x4bf/0x4f0 [dm_crypt] > > This patch fixes the issue by aligning per-bio alocated structure size. > > Reported-by: Krzysztof Kolasa <kkolasa@winsoft.pl> > Signed-off-by: Milan Broz <gmazyland@gmail.com> > --- > drivers/md/dm-crypt.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c > index 2785007..33f26a2 100644 > --- a/drivers/md/dm-crypt.c > +++ b/drivers/md/dm-crypt.c > @@ -1735,9 +1735,10 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv) > goto bad; > } > > - cc->per_bio_data_size = ti->per_bio_data_size = > - sizeof(struct dm_crypt_io) + cc->dmreq_start + > - sizeof(struct dm_crypt_request) + cc->iv_size; > + cc->per_bio_data_size = ALIGN(sizeof(struct dm_crypt_io) + cc->dmreq_start + > + sizeof(struct dm_crypt_request) + cc->iv_size, > + ARCH_KMALLOC_MINALIGN); > + ti->per_bio_data_size = cc->per_bio_data_size; > > cc->page_pool = mempool_create_page_pool(MIN_POOL_PAGES, 0); > if (!cc->page_pool) { patch tested positive for x86 ( no oops ) and x86_64 ( continues to work OK ). thanks -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hi I would like to see the explanation, why does this patch fix it. i686 allows unaligned access for most instructions, so I wonder how could adding an alignment fix it. What is the exact cipher mode that crashes it? How can I reproduce it with cryptsetup? Is it possible that something shoots beyond the end of cc->iv_size and the alignment just masks this bug? Mikulas On Mon, 18 Aug 2014, Milan Broz wrote: > The commit > 298a9fa08a1577211d42a75e8fc073baef61e0d9 > dm crypt: use per-bio data > causes OOPS on 32bit i686 architecture > > BUG: unable to handle kernel paging request at 20000000 > IP: [<e0fe2433>] clone_endio+0x13/0xe0 [dm_mod] > ... > > [<c1257b61>] bio_endio+0x61/0x90 > [<e142476c>] crypt_dec_pending+0x8c/0xd0 [dm_crypt] > [<e142666f>] kcryptd_crypt+0x4bf/0x4f0 [dm_crypt] > > This patch fixes the issue by aligning per-bio alocated structure size. > > Reported-by: Krzysztof Kolasa <kkolasa@winsoft.pl> > Signed-off-by: Milan Broz <gmazyland@gmail.com> > --- > drivers/md/dm-crypt.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c > index 2785007..33f26a2 100644 > --- a/drivers/md/dm-crypt.c > +++ b/drivers/md/dm-crypt.c > @@ -1735,9 +1735,10 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv) > goto bad; > } > > - cc->per_bio_data_size = ti->per_bio_data_size = > - sizeof(struct dm_crypt_io) + cc->dmreq_start + > - sizeof(struct dm_crypt_request) + cc->iv_size; > + cc->per_bio_data_size = ALIGN(sizeof(struct dm_crypt_io) + cc->dmreq_start + > + sizeof(struct dm_crypt_request) + cc->iv_size, > + ARCH_KMALLOC_MINALIGN); > + ti->per_bio_data_size = cc->per_bio_data_size; > > cc->page_pool = mempool_create_page_pool(MIN_POOL_PAGES, 0); > if (!cc->page_pool) { > -- > 2.1.0 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 08/19/2014 08:37 PM, Mikulas Patocka wrote: > Hi > > I would like to see the explanation, why does this patch fix it. i686 > allows unaligned access for most instructions, so I wonder how could > adding an alignment fix it. > > What is the exact cipher mode that crashes it? How can I reproduce it with > cryptsetup? > > Is it possible that something shoots beyond the end of cc->iv_size and the > alignment just masks this bug? Hi Mikulas, TBH I did not analysed it in detail, but apparently there is 4byte more needed on 32bit arch, I checked size before and after my patch and these 4 bytes solves the problem. (I guess crypto cipher api requires alignment here but I have really no time to trace it now.) For me it crashes lrw mode for twofish (I think it uses twofish_i586 but cannot verify it now) (but see oops log posted) but probably there are more cases. If there is no other magic related, it should be easily reproducible just by running "make check" (or directly tcrypt-compat-test) from cryptsetup upstream (1.6.6 release is also fine) on 32 bit with 3.17-rc1. (I am running it with AES-NI capable CPU, quite common Lenovo nb config.) Milan > > Mikulas > > > > On Mon, 18 Aug 2014, Milan Broz wrote: > >> The commit >> 298a9fa08a1577211d42a75e8fc073baef61e0d9 >> dm crypt: use per-bio data >> causes OOPS on 32bit i686 architecture >> >> BUG: unable to handle kernel paging request at 20000000 >> IP: [<e0fe2433>] clone_endio+0x13/0xe0 [dm_mod] >> ... >> >> [<c1257b61>] bio_endio+0x61/0x90 >> [<e142476c>] crypt_dec_pending+0x8c/0xd0 [dm_crypt] >> [<e142666f>] kcryptd_crypt+0x4bf/0x4f0 [dm_crypt] >> >> This patch fixes the issue by aligning per-bio alocated structure size. >> >> Reported-by: Krzysztof Kolasa <kkolasa@winsoft.pl> >> Signed-off-by: Milan Broz <gmazyland@gmail.com> >> --- >> drivers/md/dm-crypt.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c >> index 2785007..33f26a2 100644 >> --- a/drivers/md/dm-crypt.c >> +++ b/drivers/md/dm-crypt.c >> @@ -1735,9 +1735,10 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv) >> goto bad; >> } >> >> - cc->per_bio_data_size = ti->per_bio_data_size = >> - sizeof(struct dm_crypt_io) + cc->dmreq_start + >> - sizeof(struct dm_crypt_request) + cc->iv_size; >> + cc->per_bio_data_size = ALIGN(sizeof(struct dm_crypt_io) + cc->dmreq_start + >> + sizeof(struct dm_crypt_request) + cc->iv_size, >> + ARCH_KMALLOC_MINALIGN); >> + ti->per_bio_data_size = cc->per_bio_data_size; >> >> cc->page_pool = mempool_create_page_pool(MIN_POOL_PAGES, 0); >> if (!cc->page_pool) { >> -- >> 2.1.0 >> -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, 19 Aug 2014, Milan Broz wrote: > > On 08/19/2014 08:37 PM, Mikulas Patocka wrote: > > Hi > > > > I would like to see the explanation, why does this patch fix it. i686 > > allows unaligned access for most instructions, so I wonder how could > > adding an alignment fix it. > > > > What is the exact cipher mode that crashes it? How can I reproduce it with > > cryptsetup? > > > > Is it possible that something shoots beyond the end of cc->iv_size and the > > alignment just masks this bug? > > Hi Mikulas, > > TBH I did not analysed it in detail, but apparently there is 4byte more needed > on 32bit arch, I checked size before and after my patch and these > 4 bytes solves the problem. (I guess crypto cipher api requires alignment here but > I have really no time to trace it now.) > > For me it crashes lrw mode for twofish (I think it uses twofish_i586 but cannot verify it now) > (but see oops log posted) but probably there are more cases. > > If there is no other magic related, it should be easily reproducible just by running > "make check" (or directly tcrypt-compat-test) from cryptsetup upstream (1.6.6 release is also fine) > on 32 bit with 3.17-rc1. > > (I am running it with AES-NI capable CPU, quite common Lenovo nb config.) > > Milan Hi The bug is caused by this: Here we calculate dm_req_start and allocate cc->dmreq_start + sizeof(struct dm_crypt_request) + cc->iv_size bytes. cc->dmreq_start += crypto_ablkcipher_alignmask(any_tfm(cc)) & ~(crypto_tfm_ctx_alignment() - 1); cc->req_pool = mempool_create_kmalloc_pool(MIN_IOS, cc->dmreq_start + sizeof(struct dm_crypt_request) + cc->iv_size); cc->per_bio_data_size = ti->per_bio_data_size = sizeof(struct dm_crypt_io) + cc->dmreq_start + sizeof(struct dm_crypt_request) + cc->iv_size; Here, we take the end of struct dm_crypt_request (it may not be aligned), align it and use it as iv: static u8 *iv_of_dmreq(struct crypt_config *cc, struct dm_crypt_request *dmreq) { return (u8 *)ALIGN((unsigned long)(dmreq + 1), crypto_ablkcipher_alignmask(any_tfm(cc)) + 1); } The result is that iv may point beyond the end of allocated space. This bug is very old, it existed there from 3a7f6c990ad04e6f576a159876c602d14d6f7fef (2.6.25). The bug was masked by the fact that kmalloc rounds up size to the next power of two. The new changes in dm-crypt in 3.17-rc1 remove this rounding and thus the bug started to show up. I think we also need new debug option for kmalloc that will catch writes beyond the end of kmalloc memory like this. (the current slab debug doesn't catch it because it checks for writes beyond the end of the slab chunk, which was already padded to the next power of 2). Mikulas -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index 2785007..33f26a2 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -1735,9 +1735,10 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv) goto bad; } - cc->per_bio_data_size = ti->per_bio_data_size = - sizeof(struct dm_crypt_io) + cc->dmreq_start + - sizeof(struct dm_crypt_request) + cc->iv_size; + cc->per_bio_data_size = ALIGN(sizeof(struct dm_crypt_io) + cc->dmreq_start + + sizeof(struct dm_crypt_request) + cc->iv_size, + ARCH_KMALLOC_MINALIGN); + ti->per_bio_data_size = cc->per_bio_data_size; cc->page_pool = mempool_create_page_pool(MIN_POOL_PAGES, 0); if (!cc->page_pool) {
The commit 298a9fa08a1577211d42a75e8fc073baef61e0d9 dm crypt: use per-bio data causes OOPS on 32bit i686 architecture BUG: unable to handle kernel paging request at 20000000 IP: [<e0fe2433>] clone_endio+0x13/0xe0 [dm_mod] ... [<c1257b61>] bio_endio+0x61/0x90 [<e142476c>] crypt_dec_pending+0x8c/0xd0 [dm_crypt] [<e142666f>] kcryptd_crypt+0x4bf/0x4f0 [dm_crypt] This patch fixes the issue by aligning per-bio alocated structure size. Reported-by: Krzysztof Kolasa <kkolasa@winsoft.pl> Signed-off-by: Milan Broz <gmazyland@gmail.com> --- drivers/md/dm-crypt.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)