commit 7be04e75bc64dc288e51f83495d89135a8c9d4d7
Author: Patrick McHardy <[email protected]>
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 <[email protected]>
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));
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
commit 37a885564f7a0b687e0330fc6a43b0b2cddca7ea
Author: Herbert Xu <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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);
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.
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 :)
On Tue, Apr 29, 2008 at 09:59:32PM +0800, Herbert Xu ([email protected]) 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
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Tue, Apr 29, 2008 at 10:45:03PM +0800, Herbert Xu ([email protected]) 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