[RFC XFRM]: esp: fix scatterlist of out bounds access with crypto_eseqiv

[RFC XFRM]: esp: fix scatterlist of out bounds access with crypto_eseqiv

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

I ran into occasional BUGs in scatterlist.h, which turned
out the be caused by accessing an uninitialized scatterlist
entry from eseqiv. I'm not sure whether this patch is correct
since I'm seeing invalid packets with and without this patch
(probably related to HIFN though) and I don't understand why
scatterwalk_sg_next() returns either a scatterlist or a
struct page dependant on the length, but at least it fixes
the BUG() for me :)


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3277 bytes --]

commit 7be04e75bc64dc288e51f83495d89135a8c9d4d7
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Apr 28 19:24:23 2008 +0200

    [XFRM]: esp: fix scatterlist of out bounds access with crypto_eseqiv
    
    ESP allocates a src scatterlist for the exact amount of scatterlist
    entries. In the IPSec case (IV is directly before the plaintext data)
    eseqiv_chain() calls scatterwalk_sg_next(), which advances the
    scatterlist by one, pointing to uninitalized memory. When sg->length
    is zero, it returns sg_page(sg), which BUGs with DEBUG_SG enabled
    because the magic number is invalid.
    
    Allocate and initialize a spare scatterlist entry in esp4/esp6 to fix this.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index 4e73e57..6803b90 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -139,7 +139,7 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 		goto error;
 	nfrags = err;
 
-	tmp = esp_alloc_tmp(aead, nfrags + 1);
+	tmp = esp_alloc_tmp(aead, nfrags + 2);
 	if (!tmp)
 		goto error;
 
@@ -201,7 +201,7 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 	esph->spi = x->id.spi;
 	esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output);
 
-	sg_init_table(sg, nfrags);
+	sg_init_table(sg, nfrags + 1);
 	skb_to_sgvec(skb, sg,
 		     esph->enc_data + crypto_aead_ivsize(aead) - skb->data,
 		     clen + alen);
@@ -347,7 +347,7 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
 	nfrags = err;
 
 	err = -ENOMEM;
-	tmp = esp_alloc_tmp(aead, nfrags + 1);
+	tmp = esp_alloc_tmp(aead, nfrags + 2);
 	if (!tmp)
 		goto out;
 
@@ -364,7 +364,7 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
 	/* Get ivec. This can be wrong, check against another impls. */
 	iv = esph->enc_data;
 
-	sg_init_table(sg, nfrags);
+	sg_init_table(sg, nfrags + 1);
 	skb_to_sgvec(skb, sg, sizeof(*esph) + crypto_aead_ivsize(aead), elen);
 	sg_init_one(asg, esph, sizeof(*esph));
 
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index c6bb4c6..d00c74c 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -163,7 +163,7 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 		goto error;
 	nfrags = err;
 
-	tmp = esp_alloc_tmp(aead, nfrags + 1);
+	tmp = esp_alloc_tmp(aead, nfrags + 2);
 	if (!tmp)
 		goto error;
 
@@ -190,7 +190,7 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 	esph->spi = x->id.spi;
 	esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output);
 
-	sg_init_table(sg, nfrags);
+	sg_init_table(sg, nfrags + 1);
 	skb_to_sgvec(skb, sg,
 		     esph->enc_data + crypto_aead_ivsize(aead) - skb->data,
 		     clen + alen);
@@ -298,7 +298,7 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
 	}
 
 	ret = -ENOMEM;
-	tmp = esp_alloc_tmp(aead, nfrags + 1);
+	tmp = esp_alloc_tmp(aead, nfrags + 2);
 	if (!tmp)
 		goto out;
 
@@ -315,7 +315,7 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
 	/* Get ivec. This can be wrong, check against another impls. */
 	iv = esph->enc_data;
 
-	sg_init_table(sg, nfrags);
+	sg_init_table(sg, nfrags + 1);
 	skb_to_sgvec(skb, sg, sizeof(*esph) + crypto_aead_ivsize(aead), elen);
 	sg_init_one(asg, esph, sizeof(*esph));
 

Re: [RFC XFRM]: esp: fix scatterlist of out bounds access with crypto_eseqiv

[Herbert Xu]

Hi Patrick:

On Mon, Apr 28, 2008 at 08:55:21PM +0200, Patrick McHardy wrote:
> I ran into occasional BUGs in scatterlist.h, which turned
> out the be caused by accessing an uninitialized scatterlist
> entry from eseqiv. I'm not sure whether this patch is correct
> since I'm seeing invalid packets with and without this patch
> (probably related to HIFN though) and I don't understand why
> scatterwalk_sg_next() returns either a scatterlist or a
> struct page dependant on the length, but at least it fixes
> the BUG() for me :)

Can you attach the BUG output please?

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC XFRM]: esp: fix scatterlist of out bounds access with crypto_eseqiv

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

Herbert Xu wrote:
> Hi Patrick:
> 
> On Mon, Apr 28, 2008 at 08:55:21PM +0200, Patrick McHardy wrote:
>> I ran into occasional BUGs in scatterlist.h, which turned
>> out the be caused by accessing an uninitialized scatterlist
>> entry from eseqiv. I'm not sure whether this patch is correct
>> since I'm seeing invalid packets with and without this patch
>> (probably related to HIFN though) and I don't understand why
>> scatterwalk_sg_next() returns either a scatterlist or a
>> struct page dependant on the length, but at least it fixes
>> the BUG() for me :)
> 
> Can you attach the BUG output please?


I've attached two traces, the one from eseqiv and a similar
one from authenc (I've manually overriden eseqiv by chainiv
to test whether its responsible for the broken packets I was
seeing, which turned out to be the case. I'll look into that).


[-- Attachment #2: eseqiv.oops --]
[-- Type: text/plain, Size: 2471 bytes --]

------------[ cut here ]------------
kernel BUG at include/linux/scatterlist.h:96!
invalid opcode: 0000 [#1] PREEMPT DEBUG_PAGEALLOC
Modules linked in: authenc esp4 aead xfrm4_mode_tunnel sha1_generic hmac crypto_hash cryptomgr]

Pid: 1548, comm: ping Not tainted (2.6.25 #75)
EIP: 0060:[<dc81e69b>] EFLAGS: 00010213 CPU: 0
EIP is at eseqiv_chain+0x21/0x90 [crypto_blkcipher]
EAX: 0000006c EBX: dba27da8 ECX: 00000001 EDX: dba27e88
ESI: 00374300 EDI: dba27da8 EBP: daa32ba0 ESP: daa32b9c
 DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
Process ping (pid: 1548, ti=daa32000 task=da9f4000 task.ti=daa32000)
Stack: 00000010 daa32bf8 dc81e905 daa32bd2 dba27e08 dba27d70 db9ea930 dba27e20 
       dc92d4fa dba27d40 dba27e70 dba27e70 c0153e57 dba1848c dba1849c dba1849c 
       0000048c dba1849c 00000060 daa32bf8 db9ea900 dba27d70 00000060 daa32c08 
Call Trace:
 [<dc81e905>] ? eseqiv_givencrypt+0x19c/0x2c1 [crypto_blkcipher]
 [<dc92d4fa>] ? crypto_authenc_givencrypt_done+0x0/0x24 [authenc]
 [<c0153e57>] ? __slab_alloc+0x389/0x3f5
 [<dc81ea9e>] ? eseqiv_givencrypt_first+0x4a/0x50 [crypto_blkcipher]
 [<dc92d649>] ? crypto_authenc_givencrypt+0x65/0x80 [authenc]
 [<dc92a9d1>] ? esp_output+0x283/0x2ae [esp4]
 [<c025878e>] ? xfrm_output_resume+0x24a/0x339
 [<c025888a>] ? xfrm_output2+0xd/0xf
 [<c0258954>] ? xfrm_output+0xc8/0xd4
 [<c0251efe>] ? xfrm4_output+0xe/0x10
 [<c022dbea>] ? ip_local_out+0x18/0x1b
 [<c022df1b>] ? ip_push_pending_frames+0x24f/0x2b6
 [<c0244297>] ? raw_sendmsg+0x53f/0x5b7
 [<c024a873>] ? inet_sendmsg+0x3b/0x48
 [<c020f230>] ? sock_sendmsg+0xc9/0xe0
 [<c012841f>] ? autoremove_wake_function+0x0/0x30
 [<c01146f5>] ? __wake_up_common+0x2e/0x54
 [<c01168fe>] ? __wake_up+0x1d/0x3d
 [<c01e132a>] ? n_tty_receive_buf+0xd2f/0xd7a
 [<c01b04fa>] ? copy_from_user+0x2c/0x4f
 [<c021523d>] ? verify_iovec+0x40/0x6f
 [<c020f394>] ? sys_sendmsg+0x14d/0x1a8
 [<c0115a80>] ? hrtick_set+0x7b/0xcb
 [<c013b439>] ? find_lock_page+0x28/0xb1
 [<c013d1ff>] ? filemap_fault+0x1ee/0x345
 [<c013b350>] ? unlock_page+0x24/0x27
 [<c014566e>] ? __do_fault+0x2cd/0x307
 [<c0263bed>] ? __lock_text_start+0x25/0x27
 [<c0160955>] ? vfs_ioctl+0x55/0x67
 [<c0210086>] ? sys_socketcall+0x146/0x15e
 [<c01038c5>] ? sysenter_past_esp+0x6a/0x91
 =======================
Code: 10 89 f2 ff 53 18 5b 5e 5d c3 55 85 c9 89 e5 53 89 c3 74 2b 8b 42 0c 83 c2 18 01 43 0c 8 
EIP: [<dc81e69b>] eseqiv_chain+0x21/0x90 [crypto_blkcipher] SS:ESP 0068:daa32b9c
---[ end trace 99e8b865243b3a33 ]---


[-- Attachment #3: authenc.oops --]
[-- Type: text/plain, Size: 2342 bytes --]

Pid: 1536, comm: ping Not tainted (2.6.25 #74)
EIP: 0060:[<dc92d04b>] EFLAGS: 00010213 CPU: 0
EIP is at authenc_chain+0x21/0x90 [authenc]
EAX: 0000006c EBX: c033df20 ECX: 00000001 EDX: db99dcd0
ESI: db99dcb8 EDI: dba228ec EBP: c033df00 ESP: c033defc
 DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
Process ping (pid: 1536, ti=c033d000 task=da9ee380 task.ti=daa35000)
Stack: 1ba22000 c033df5c dc92d223 00000000 db99dc00 000008fc da9e5150 00000010 
       c1001000 87654321 c1375440 000008ec 0000007c 00000000 00000000 87654321 
       00000002 00000000 00000000 00000000 00000000 db99dc68 db9fb240 dbb61720 
Call Trace:
 [<dc92d223>] ? crypto_authenc_genicv+0xcb/0x109 [authenc]
 [<dc92d511>] ? crypto_authenc_givencrypt_done+0x17/0x24 [authenc]
 [<dc844a63>] ? hifn_process_ready+0x22f/0x237 [hifn_795x]
 [<dc845722>] ? hifn_check_for_completion+0x4d/0xa6 [hifn_795x]
 [<c011fee0>] ? run_timer_softirq+0x14/0x176
 [<dc845785>] ? hifn_tasklet_callback+0xa/0xc [hifn_795x]
 [<c011d046>] ? tasklet_action+0x3f/0x66
 [<c011d230>] ? __do_softirq+0x38/0x7a
 [<c0105a5f>] ? do_softirq+0x3e/0x71
 [<c0139e1f>] ? handle_fasteoi_irq+0x0/0xbf
 [<c011d17c>] ? irq_exit+0x2c/0x65
 [<c0105b27>] ? do_IRQ+0x95/0xaa
 [<c01042b7>] ? common_interrupt+0x23/0x28
 [<c0262ad2>] ? schedule_timeout+0x1/0x91
 [<c0215954>] ? __skb_recv_datagram+0x15f/0x1b7
 [<c012841f>] ? autoremove_wake_function+0x0/0x30
 [<c02159cc>] ? skb_recv_datagram+0x20/0x25
 [<c0243c88>] ? raw_recvmsg+0x5e/0x12e
 [<c021050c>] ? sock_common_recvmsg+0x31/0x4a
 [<c020f14f>] ? sock_recvmsg+0xd0/0xe8
 [<c012841f>] ? autoremove_wake_function+0x0/0x30
 [<c01e132a>] ? n_tty_receive_buf+0xd2f/0xd7a
 [<c01b04fa>] ? copy_from_user+0x2c/0x4f
 [<c021523d>] ? verify_iovec+0x40/0x6f
 [<c020fb97>] ? sys_recvmsg+0xf2/0x17f
 [<c0115a80>] ? hrtick_set+0x7b/0xcb
 [<c0103611>] ? do_notify_resume+0x6ef/0x703
 [<c013b350>] ? unlock_page+0x24/0x27
 [<c014566e>] ? __do_fault+0x2cd/0x307
 [<c0263bed>] ? __lock_text_start+0x25/0x27
 [<c0160955>] ? vfs_ioctl+0x55/0x67
 [<c0210092>] ? sys_socketcall+0x152/0x15e
 [<c01038c5>] ? sysenter_past_esp+0x6a/0x91
 =======================
Code: d8 e8 c6 70 82 e3 5b 5e 5d c3 55 85 c9 89 e5 53 89 c3 74 2b 8b 42 0c 83 c2 18 01 43 0c 8 
EIP: [<dc92d04b>] authenc_chain+0x21/0x90 [authenc] SS:ESP 0068:c033defc
Kernel panic - not syncing: Fatal exception in interrupt

Re: [RFC XFRM]: esp: fix scatterlist of out bounds access with crypto_eseqiv

[Herbert Xu]

On Tue, Apr 29, 2008 at 07:09:39AM +0200, Patrick McHardy wrote:
> 
> I've attached two traces, the one from eseqiv and a similar
> one from authenc (I've manually overriden eseqiv by chainiv
> to test whether its responsible for the broken packets I was
> seeing, which turned out to be the case. I'll look into that).

Thanks, looks like I left out the sg_is_last check in restoring
adding scatterwalk_sg_next.  Worse yet, eseqiv doesn't even
encrypt the last block.  It's a good thing the hifn driver doesn't
work yet :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
commit 37a885564f7a0b687e0330fc6a43b0b2cddca7ea
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Tue Apr 29 21:53:52 2008 +0800

    [CRYPTO] api: Fix scatterwalk_sg_chain
    
    When I backed out of using the generic sg chaining (as it isn't currently
    portable) and introduced scatterwalk_sg_chain/scatterwalk_sg_next I left
    out the sg_is_last check in the latter.  This causes it to potentially
    dereference beyond the end of the sg array.
    
    As most uses of scatterwalk_sg_next are bound by an overall length, this
    only affected the chaining code in authenc and eseqiv. Thanks to Patrick
    McHardy for identifying this problem.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/include/crypto/scatterwalk.h b/include/crypto/scatterwalk.h
index 224658b..833d208 100644
--- a/include/crypto/scatterwalk.h
+++ b/include/crypto/scatterwalk.h
@@ -57,10 +57,14 @@ static inline void scatterwalk_sg_chain(struct scatterlist *sg1, int num,
 					struct scatterlist *sg2)
 {
 	sg_set_page(&sg1[num - 1], (void *)sg2, 0, 0);
+	sg1[num - 1].page_link &= ~0x02;
 }
 
 static inline struct scatterlist *scatterwalk_sg_next(struct scatterlist *sg)
 {
+	if (sg_is_last(sg))
+		return NULL;
+
 	return (++sg)->length ? sg : (void *)sg_page(sg);
 }
 
commit 325709763dffb453c6a710ca3dfe184e8909f27a
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Tue Apr 29 21:57:01 2008 +0800

    [CRYPTO] eseqiv: Fix off-by-one encryption
    
    After attaching the IV to the head during encryption, eseqiv does not
    increase the encryption length by that amount.  As such the last block
    of the actual plain text will be left unencrypted.
    
    Fortunately the only user of this code hifn currently crashes so this
    shouldn't affect anyone :)
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/eseqiv.c b/crypto/eseqiv.c
index b14f14e..881d309 100644
--- a/crypto/eseqiv.c
+++ b/crypto/eseqiv.c
@@ -136,7 +136,8 @@ static int eseqiv_givencrypt(struct skcipher_givcrypt_request *req)
 	}
 
 	ablkcipher_request_set_crypt(subreq, reqctx->src, dst,
-				     req->creq.nbytes, req->creq.info);
+				     req->creq.nbytes + ivsize,
+				     req->creq.info);
 
 	memcpy(req->creq.info, ctx->salt, ivsize);
 

Re: [RFC XFRM]: esp: fix scatterlist of out bounds access with crypto_eseqiv

[Patrick McHardy]

Herbert Xu wrote:
> On Tue, Apr 29, 2008 at 07:09:39AM +0200, Patrick McHardy wrote:
>   
>> I've attached two traces, the one from eseqiv and a similar
>> one from authenc (I've manually overriden eseqiv by chainiv
>> to test whether its responsible for the broken packets I was
>> seeing, which turned out to be the case. I'll look into that).
>>     
>
> Thanks, looks like I left out the sg_is_last check in restoring
> adding scatterwalk_sg_next.  Worse yet, eseqiv doesn't even
> encrypt the last block.  It's a good thing the hifn driver doesn't
> work yet :)

Thanks for looking into this, the eseqiv problem is exactly the one
I'm seeing :) I'll test your patch and let you know the results.

Re: [RFC XFRM]: esp: fix scatterlist of out bounds access with crypto_eseqiv

[Patrick McHardy]

Patrick McHardy wrote:
> Herbert Xu wrote:
>> On Tue, Apr 29, 2008 at 07:09:39AM +0200, Patrick McHardy wrote:
>>  
>>> I've attached two traces, the one from eseqiv and a similar
>>> one from authenc (I've manually overriden eseqiv by chainiv
>>> to test whether its responsible for the broken packets I was
>>> seeing, which turned out to be the case. I'll look into that).
>>>     
>>
>> Thanks, looks like I left out the sg_is_last check in restoring
>> adding scatterwalk_sg_next.  Worse yet, eseqiv doesn't even
>> encrypt the last block.  It's a good thing the hifn driver doesn't
>> work yet :)
>
> Thanks for looking into this, the eseqiv problem is exactly the one
> I'm seeing :) I'll test your patch and let you know the results.

Works perfectly, thanks again :) This fixes my (hopefully)
second to last problem with HIFN :)

Re: [RFC XFRM]: esp: fix scatterlist of out bounds access with crypto_eseqiv

[Evgeniy Polyakov]

On Tue, Apr 29, 2008 at 09:59:32PM +0800, Herbert Xu (herbert@gondor.apana.org.au) wrote:
> It's a good thing the hifn driver doesn't work yet :)

Hey, it works :))
Just not always, which is great at attracting Patrick and problems.

-- 
	Evgeniy Polyakov

Re: [RFC XFRM]: esp: fix scatterlist of out bounds access with crypto_eseqiv

[Herbert Xu]

On Tue, Apr 29, 2008 at 06:21:44PM +0400, Evgeniy Polyakov wrote:
> 
> Hey, it works :))
> Just not always, which is great at attracting Patrick and problems.

I was more commenting on the bugs in the crypto layer that breaks
hifn rather than the hifn driver itself :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC XFRM]: esp: fix scatterlist of out bounds access with crypto_eseqiv

[Evgeniy Polyakov]

On Tue, Apr 29, 2008 at 10:45:03PM +0800, Herbert Xu (herbert@gondor.apana.org.au) wrote:
> On Tue, Apr 29, 2008 at 06:21:44PM +0400, Evgeniy Polyakov wrote:
> > 
> > Hey, it works :))
> > Just not always, which is great at attracting Patrick and problems.
> 
> I was more commenting on the bugs in the crypto layer that breaks
> hifn rather than the hifn driver itself :)

I was just kidding of course :)

Actually this is great that even obscure problems got fixed because of
some other problems in seems to be unrelated areas.

-- 
	Evgeniy Polyakov