[SCTP]: Always linearise packet on input

[SCTP]: Always linearise packet on input

[Herbert Xu]

Hi Dave:

[SCTP]: Always linearise packet on input

I was looking at a RHEL5 bug report involving Xen and SCTP
(https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=212550).
It turns out that SCTP wasn't written to handle skb fragments at
all.  The absence of any calls to skb_may_pull is testament to
that.

It just so happens that Xen creates fragmented packets more often
than other scenarios (header & data split when going from domU to
dom0).  That's what caused this bug to show up.

Until someone has the time sits down and audits the entire net/sctp
directory, here is a conservative and safe solution that simply
linearises all packets on input.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Sridhar, could you please organise an audit of SCTP to make sure
that it deals with skb fragments (both pages and frag_list) in a
safe manner? Basically before you do an skb_pull or access skb->data
you must ensure that the data you want to access is in the head,
usually by doing skb_may_pull.  Of course, for large payloads it
helps if you can avoid pulling it into the head.

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
--
diff --git a/net/sctp/input.c b/net/sctp/input.c
index 64f6301..99c0501 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -135,6 +135,9 @@ int sctp_rcv(struct sk_buff *skb)
 
 	SCTP_INC_STATS_BH(SCTP_MIB_INSCTPPACKS);
 
+	if (skb_linearize(skb))
+		goto discard_it;
+
 	sh = (struct sctphdr *) skb->h.raw;
 
 	/* Pull up the IP and SCTP headers. */

Re: [SCTP]: Always linearise packet on input

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 30 Oct 2006 18:11:28 +1100

> [SCTP]: Always linearise packet on input
 ...
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

I'll apply this, thanks a lot.

> Sridhar, could you please organise an audit of SCTP to make sure
> that it deals with skb fragments (both pages and frag_list) in a
> safe manner? Basically before you do an skb_pull or access skb->data
> you must ensure that the data you want to access is in the head,
> usually by doing skb_may_pull.  Of course, for large payloads it
> helps if you can avoid pulling it into the head.

Of course, you mean "pskb_may_pull" here.
:-)

Re: [SCTP]: Always linearise packet on input

[Sridhar Samudrala]

On Sun, 2006-10-29 at 23:46 -0800, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Mon, 30 Oct 2006 18:11:28 +1100
> 
> > [SCTP]: Always linearise packet on input
>  ...
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> I'll apply this, thanks a lot.
> 
> > Sridhar, could you please organise an audit of SCTP to make sure
> > that it deals with skb fragments (both pages and frag_list) in a
> > safe manner? Basically before you do an skb_pull or access skb->data
> > you must ensure that the data you want to access is in the head,
> > usually by doing skb_may_pull.  Of course, for large payloads it
> > helps if you can avoid pulling it into the head.
> 
> Of course, you mean "pskb_may_pull" here.

I think we currently assume atleast the SCTP header and the data
chunk header to be in the skb head.
But we do handle skbs with data in the frag_list.
Not sure about skb's with paged fragments.

Does XEN use frag_list or frags array?
Is there a simple way to simulate incoming packets with transport
headers and data in skb's frag_list/pages without having to use XEN.

Thanks
Sridhar

Re: [SCTP]: Always linearise packet on input

[Herbert Xu]

Hi Sridhar:

On Mon, Oct 30, 2006 at 05:31:24PM -0800, Sridhar Samudrala wrote:
> 
> I think we currently assume atleast the SCTP header and the data
> chunk header to be in the skb head.
> But we do handle skbs with data in the frag_list.
> Not sure about skb's with paged fragments.

You can't assume the chunk header to be in the head.  Think about what
happens when some malicious person sends you a fragmented SCTP packet.

> Does XEN use frag_list or frags array?

Xen creates paged frags in domU=>dom0 or domU=>domU traffic.
Of course frag_list can always occur as a result of IP fragmentation.

> Is there a simple way to simulate incoming packets with transport
> headers and data in skb's frag_list/pages without having to use XEN.

You can use IP fragments to create them.

But the important thing is to work through the code.  Basically wherever
you see things like skb_pull/skb->data without a preceding pskb_may_pull
call, then you have a problem.

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: [SCTP]: Always linearise packet on input

[Vlad Yasevich]

Herbert Xu wrote:
> Hi Sridhar:
> 
> On Mon, Oct 30, 2006 at 05:31:24PM -0800, Sridhar Samudrala wrote:
>> I think we currently assume atleast the SCTP header and the data
>> chunk header to be in the skb head.
>> But we do handle skbs with data in the frag_list.
>> Not sure about skb's with paged fragments.
> 
> You can't assume the chunk header to be in the head.  Think about what
> happens when some malicious person sends you a fragmented SCTP packet.
> 
>> Does XEN use frag_list or frags array?
> 
> Xen creates paged frags in domU=>dom0 or domU=>domU traffic.
> Of course frag_list can always occur as a result of IP fragmentation.
> 
>> Is there a simple way to simulate incoming packets with transport
>> headers and data in skb's frag_list/pages without having to use XEN.
> 
> You can use IP fragments to create them.
> 
> But the important thing is to work through the code.  Basically wherever
> you see things like skb_pull/skb->data without a preceding pskb_may_pull
> call, then you have a problem.

Wouldn't this in the end be equivalent to skb_linearize()?  I am trying to
think of a way do things without reallocating too much memory.

Yes, SCTP is really broken with regard to fragmented skbs.  In fact, I
have a test case that will crash the lksctp at will when receiving an IP fragmented
message.

The reason pskb_may_pull() is not a great solution IMO, is because we
may end up doing very large orders of allocations if someone decided to use 9000 MTU
on the first hop.  I can see things going bad on loopback with 16K MTU as well.

-vlad

Re: [SCTP]: Always linearise packet on input

[Sridhar Samudrala]

On Tue, 2006-10-31 at 09:36 -0500, Vlad Yasevich wrote:
> Herbert Xu wrote:
> > Hi Sridhar:
> > 
> > On Mon, Oct 30, 2006 at 05:31:24PM -0800, Sridhar Samudrala wrote:
> >> I think we currently assume atleast the SCTP header and the data
> >> chunk header to be in the skb head.
> >> But we do handle skbs with data in the frag_list.
> >> Not sure about skb's with paged fragments.
> > 
> > You can't assume the chunk header to be in the head.  Think about what
> > happens when some malicious person sends you a fragmented SCTP packet.
> > 
> >> Does XEN use frag_list or frags array?
> > 
> > Xen creates paged frags in domU=>dom0 or domU=>domU traffic.
> > Of course frag_list can always occur as a result of IP fragmentation.
> > 
> >> Is there a simple way to simulate incoming packets with transport
> >> headers and data in skb's frag_list/pages without having to use XEN.
> > 
> > You can use IP fragments to create them.
> > 
> > But the important thing is to work through the code.  Basically wherever
> > you see things like skb_pull/skb->data without a preceding pskb_may_pull
> > call, then you have a problem.
> 
> Wouldn't this in the end be equivalent to skb_linearize()?  I am trying to
> think of a way do things without reallocating too much memory.
It will be equivalent only if you pass a skb->data_len as the length to 
pskb_may_pull(). However, pskb_may_pull() is generally called with much 
smaller lengths equal to a specific header length and it tries to copy
the header from the fragments to skb->tail if there is enough room.

> 
> Yes, SCTP is really broken with regard to fragmented skbs.  In fact, I
> have a test case that will crash the lksctp at will when receiving an IP fragmented
> message.

Because we support path mtu, we should not see ip fragmentation in general although 
it is possible if there is a change in path mtu during a transfer. So i guess you may
be trying your test with path mtu disabled and with very small MTUs(< 500) or 
a packet with bundled chunks.

> 
> The reason pskb_may_pull() is not a great solution IMO, is because we
> may end up doing very large orders of allocations if someone decided to use 9000 MTU
> on the first hop.  I can see things going bad on loopback with 16K MTU as well.
pskb_may_pull() should be used only to pull in the headers, not the
entire message to the skb head. But with small messages that are
bundled, this may get complicated.

Thanks
Sridhar

Re: [SCTP]: Always linearise packet on input

[Vlad Yasevich]

Sridhar Samudrala wrote:
> On Tue, 2006-10-31 at 09:36 -0500, Vlad Yasevich wrote:
>> Herbert Xu wrote:
>>> Hi Sridhar:
>>>
>>> On Mon, Oct 30, 2006 at 05:31:24PM -0800, Sridhar Samudrala wrote:
>>>> I think we currently assume atleast the SCTP header and the data
>>>> chunk header to be in the skb head.
>>>> But we do handle skbs with data in the frag_list.
>>>> Not sure about skb's with paged fragments.
>>> You can't assume the chunk header to be in the head.  Think about what
>>> happens when some malicious person sends you a fragmented SCTP packet.
>>>
>>>> Does XEN use frag_list or frags array?
>>> Xen creates paged frags in domU=>dom0 or domU=>domU traffic.
>>> Of course frag_list can always occur as a result of IP fragmentation.
>>>
>>>> Is there a simple way to simulate incoming packets with transport
>>>> headers and data in skb's frag_list/pages without having to use XEN.
>>> You can use IP fragments to create them.
>>>
>>> But the important thing is to work through the code.  Basically wherever
>>> you see things like skb_pull/skb->data without a preceding pskb_may_pull
>>> call, then you have a problem.
>> Wouldn't this in the end be equivalent to skb_linearize()?  I am trying to
>> think of a way do things without reallocating too much memory.
> It will be equivalent only if you pass a skb->data_len as the length to 
> pskb_may_pull(). However, pskb_may_pull() is generally called with much 
> smaller lengths equal to a specific header length and it tries to copy
> the header from the fragments to skb->tail if there is enough room.

Well, this stuff accumulates eventually.  Also what happens if the multiple
chunks are bundled and fragmented?  You end up doing pskb_may_pull() on
the whole first chunk which can cause reallocation, which gets very expensive.

> 
>> Yes, SCTP is really broken with regard to fragmented skbs.  In fact, I
>> have a test case that will crash the lksctp at will when receiving an IP fragmented
>> message.
> 
> Because we support path mtu, we should not see ip fragmentation in general although 
> it is possible if there is a change in path mtu during a transfer. So i guess you may
> be trying your test with path mtu disabled and with very small MTUs(< 500) or 
> a packet with bundled chunks.

The problem happens with the MTU of 512.  If I run over loopback, I can get it to happen
when dropping mtu to 1500.   The test simulates real world wireless network used between
cellular towers.

> 
>> The reason pskb_may_pull() is not a great solution IMO, is because we
>> may end up doing very large orders of allocations if someone decided to use 9000 MTU
>> on the first hop.  I can see things going bad on loopback with 16K MTU as well.
> pskb_may_pull() should be used only to pull in the headers, not the
> entire message to the skb head. But with small messages that are
> bundled, this may get complicated.

Yes,  this is the problem that I can reproduce.  If I end up bundling multiple
messages, at the same time as IP mtu drops, we end up with IP fragmentation and
a crash on the receiver.

My temporary solution has also been a skb_linearize(), but I've been trying to
get away from that.

-vlad