Subject: question about sctp_grub_through_socket_buffer

From: Xiaodan Tang <[email-masked]>
Date: Wed, 9 Apr 2003 00:31:25 -0400

Hello,

I am integrating SCTP into our tcpip stack (with is a NetBSD 1.6 base).
The sctp source is took from kame cvs.

The peeloff test seems can only run one pass. Starting from second
pass, reading from peeled off fd1, didn't return me proper data.
(it actually return me data for next buffer2 ...)

After poking around, I found the inp->sctp_vtag_last didn't seems
changed during first/second peeloff.  In 2nd time, when we are in
sctp_grub_through_socket_buffer(), it think the first mbuf is not
the association we want...

My question is, in the sctp_grub_through_socket_buffer(), if we actually
moved first mbuf to new socket, shouldn't we also reposition
inp->sctp_vtag_last ? The "reposition" code at the end of that function
didn't seems goes in, cause tcb->sctp_ep->sctp_vtag_last seems 0
at the moment, is this normal ? (Note even if we enter the reposition
code there, inp->sctp_vtag_last still not moved, or is it going to be 
reposition somewhere else ?)

Xiaodan Tang
R&D QNX Software Systems Ltd.

---

From: "Xiaodan Tang" <[email-masked]>
Date: Sun, 13 Apr 2003 00:03:08 -0400

Hello,

I am running peeloff test on a NetBSD based stack over 
localhost (peel_client -h 127.0.0.1 -p <port>), and the
test faild on server's 2nd pass where it detected a checksum
error in the received data.

I trace the problem in to 2 problem:

1) in sctp_grub_through_socket_buffer(), after we moved
   some data into new socket, we try to "re-position" the
   sctp_vtag_last. We are doing:

   if (tcb->sctp_ep->sctp_vtag_last == asoc->my_vtag)

   This didn't seems right to me. The tcb->sctp_ep is 
   already pointed to new_inp from sctp_get_peeloff(),
   it's sctp_vtag_last usually is 0, and since we just moved
   a lot of asoc->my_vtag buffer into new socket, 
   tcb->sctp_ep->sctp_vtag_last should just assigned to
   asoc->my_vtag. I changed these to:

   if (new_sb->sb_mb)
      tcb->sctp_ep->sctp_vtag_last = asoc->my_vtag;
   if (inp->sctp_vtag_last == asoc->my_vtag) {
       inp->sctp_vtag_last = 0;

       /* scan the old_sb->sb_mb, find the correct vtag, and
           assign it to inp->sctp_vtag_last.
       */
   }

2) in sctp_usrreq() we call sctp_usr_recvd() with flag = 0.
    this makes sctp_usr_recvd()'s code for re-position
    sctp_vtag_last won't be executed. (it depends on
    flag & MSG_EOR).

    Change it to:

       sctp_usr_recvd(so, (int)nam);

With fix 1) and 2), the peeloff start runing better

Xiaodan Tang

---

From: "Randall Stewart (cisco)" <[email-masked]>
Date: Mon, 14 Apr 2003 08:54:54 -0500

Xiaodan:

Thanks for looking in to this.. I have been on Holiday for a couple of weeks
and on business travel last week :-0

I will highlight your fixes and see if I can't get them submitted with
the next round of bug fixes to kame... after I get caught up :;-0

R

-- 
Randall R. Stewart
ITD
Cisco Systems Inc.
[email-masked] 815-342-5222(cell) or 815-477-2127(office)

---

From: "Xiaodan Tang" <[email-masked]>
Date: Mon, 14 Apr 2003 12:13:19 -0400

Thank you Randall,

I was just wondering if you still on the ML :)

Since we are one this, I have one more question :)

The "asocid" passed to user is actually the address of a struct sctp_pcb *,
it passed back from user through the "sctp_peeloff()" call.

We does check asocid (in sctp_findassociation_ep_asocid()) that 
it is in range of "highest_tcb" and "lowest_tcb". Other then that,
we just used it.

It looks dangrous that if the user passin a bogus asocid, and
happened to be in range, we could just panic kernel... 

-xtang

---

From: "Randall Stewart (cisco)" <[email-masked]>
Date: Mon, 14 Apr 2003 13:40:15 -0500

>I was just wondering if you still on the ML :)

Its called 1 week IETF, 2 weeks PTO and 1 week of travel.. sigh.. I am 
slowly
working through my backlog.. I hope to at least get through all my 
critical email
backlog by the end of today ... sigh...

>it passed back from user through the "sctp_peeloff()" call.

yep...

>it is in range of "highest_tcb" and "lowest_tcb". Other then that,

Well, yes.. but I also think the pointer must align so that the sctp_ep 
pointer
points back up at the inp structure...

aka
    if ((ep->highest_tcb >= asoc_id) && (ep->lowest_tcb <= asoc_id)) {
        /* it is possible lets have a look */
        rtcb = (struct sctp_tcb *)asoc_id;
        if ((rtcb->sctp_ep == ep) && rtcb->asoc.state) {
            return (rtcb);
        }
    }

So if the rtcb->sctp_ep address does NOT point back to the socket's inp 
pointer
you will get a NULL back and no association..

So there is more than just being in the range.. we may be able to check more
things.. but I figured that was enough..

>happened to be in range, we could just panic kernel... 

Look again at the above and tell me if you really think that to be true..

R

---

From: "Xiaodan Tang" <[email-masked]>
Date: Mon, 14 Apr 2003 15:17:11 -0400

----- Original Message -----
> pointer you will get a NULL back and no association..

But you already referenced "rtcb->sctp_ep" above. If the asoc_id is bogus,
that reference could causing either sigment fault (if the rtcb(asoc_id)
point to
a non-exist address), or alignment fault (if rtcb(asoc_id) is not aligned
and the
processor request alignment access), isn't it ?

-xtang

---

From: "Randall Stewart (cisco)" <[email-masked]>
Date: Mon, 14 Apr 2003 15:09:13 -0500

>processor request alignment access), isn't it ?

And why would that be true? The memory address is a valid kernel memory
address that has been used for an association. Yes I can make up a bogus
address inside the range of addresses but it MUST be a useable address.

So lets say I have:

Addr-1 - 0xc0000100
----------
.
.
------
Addr-2 - 0xc0000200
------
.
.
------
Addr-3 - 0xc00000300
-----

etc up to having used 0xc0000a00

Now the user MUST give an address between the values
of 0xc0000100 - 0xc000a00 (where our highest and lowest are).

These are valid addresses... they may not be his... so lets say the user 
makes
up not a properly aligned address but instead

0xc0000201

The memory area is completely valid.. maybe belonging to other
associations.. maybe sitting in the free pool/zone, the pooling code will
cache old allocations ....

Now if we examine this memory we CAN NOT get a segmentation fault... the
memory IS valid.. reading this is not a problem either.. we are not 
changing it..

But we are validating it..

R

---

From: "Xiaodan Tang" <[email-masked]>
Date: Tue, 15 Apr 2003 15:03:27 -0400

----- Original Message -----

> makes up not a properly aligned address but instead
>
> 0xc0000201
>
> associations.. maybe sitting in the free pool/zone, the pooling code
> will cache old allocations ....
>

If one piece is returned to pool/zone, doesn't the pool allowed to map if
off.
You seems bating all the OS (kernel) will never unmap their free memory.

Another problem is on CPUs like SH4 or ARM, you are not allowed to do
unalignment access. So you can't read a 4byte value (a pointer) from a
0xc0000201 address (the address is not on 4 byte boundery). Once you
do this (yes, even if you only read), an "Alignment Fault" will be
generated.

 I think we need more general means to verify the pointer.

-xtang

---

From: "Randall Stewart (cisco)" <[email-masked]>
Date: Tue, 15 Apr 2003 15:33:05 -0500

>You seems bating all the OS (kernel) will never unmap their free memory.
I agree.. it could happen .. but the reality is you have N number of active
associations... and thus the memory in this region is fairly current...

Most O/S's of today don't give back memory.. I am not saying it could
not happen in the future... but hey.. when we wrote this we had a LOT
of other things to worry about :->

>do this (yes, even if you only read), an "Alignment Fault" will be

Yep, I have thought of that too.. but, I must admit, since I am on a 
i386 platform
I shamelessly did not worry about it :-0

It would be worth thinking of a easy alternative.. at the time I wrote the
code it crossed my mind to have a hash of all active associations.. but this
of course would suck more memory.. which I really did not want to do...

Another thing one could do is search through all the associations... cycle
intensity diswaded me from that approach :0

So In the rush to get the rest of the things in place a quick workable 
compromise
was put in... does it have short comings... yep.. is it fast .. yep.. 
does it work with
current the current BSD .. yep..... and the alignment issue could be 
fixed with an
additional line

if ((assoc_id % 4) != 0)
    return(NULL);

As long as the data structure is word aligned.. which I think it is.. 
never bothered
to check that one either :-0

R

---

From: "Randall Stewart (cisco)" <[email-masked]>
Date: Tue, 15 Apr 2003 16:11:42 -0500

Xiaodan:

I have looked at the above .. and it is not correct.

The idea of sctp_vtag_last is it holds the vtag of the packet
currently being read in. So what does this code supposedly do?

1) It first checks that vtag_last... in case a partial read has been done
    before the sctp_peeloff() was called... if so it pulls the top chain 
of mbufs...

2) After that check, it then goes through the entire chain looking for
    matching vtags in the recv'd messages that are on queue.

3) After all are taken we do NOT want to touch vtag_last unless <1> above
    was true.. i.e. the top of the mbuf chain contained one that was moved.

The problem, I think, was NOT the if.. but the loop itself was using 
m_next to
move through the mbuf chain.. this is incorrect.. it should have used 
m_nextpkt.. this
same bug is also in sctp_usrreq.c

>       sctp_usr_recvd(so, (int)nam);
casting the mbuf chain nam to a int is not the right thing to do
either.. the sctp_usr_recvd should be called with the flag
when a full message has been removed from the rcv buffer...
You will get random execution based on the address of the mbuf
that holds the socket address.. not right I think..

Instead we will need to look at the top of what is left on the
recvbuf and then, if it is a m_pkthdr type, set the MSG-EOR falg

I will get a patch with this in place..

R