On Tue, Dec 8, 2015 at 6:40 PM, Marcelo Ricardo Leitner <marcelo.leitner@xxxxxxxxx> wrote: > On Tue, Dec 08, 2015 at 06:30:51PM +0100, Dmitry Vyukov wrote: >> On Mon, Dec 7, 2015 at 9:52 PM, Marcelo Ricardo Leitner >> <marcelo.leitner@xxxxxxxxx> wrote: >> > Em 07-12-2015 18:37, Vlad Yasevich escreveu: >> >> >> >> On 12/07/2015 02:50 PM, Marcelo Ricardo Leitner wrote: >> >>> >> >>> On Mon, Dec 07, 2015 at 02:33:52PM -0500, Vlad Yasevich wrote: >> >>>> >> >>>> On 12/07/2015 01:52 PM, Marcelo Ricardo Leitner wrote: >> >>>>> >> >>>>> Vlad, I reviewed the places on which it returns SCTP_DISPOSITION_ABORT, >> >>>>> and if I didn't miss something in there all of them either issue >> >>>>> SCTP_CMD_ASSOC_FAILED or SCTP_CMD_INIT_FAILED before returning it, thus >> >>>>> delaying DELETE_TCB and with that the asoc free. >> >>>> >> >>>> >> >>>> They delay it from the perspective of the command interpreter since the >> >>>> command >> >>>> to delete the TCB happens a little later, but status code is checked >> >>>> after all >> >>>> commands are processed and command processing doesn't change it. So the >> >>>> 'status' >> >>>> code would still be SCTP_DISPOSITION_ABORT after DELETE_TCB command was >> >>>> processed. >> >>>> So, I think we may still have an use-after-free issue here. >> >>> >> >>> >> >>> Gotcha! That's pretty much it then. From that point of view now, there >> >>> shouldn't be a case that it returns _ABORT without freeing the asoc in >> >>> the same loop. (more below) >> >>> >> >>>>> There is one place, >> >>>>> though, that may not do it that way, it's sctp_sf_abort_violation(), >> >>>>> but >> >>>>> then that code only runs if asoc is already NULL by then. >> >>>> >> >>>> >> >>>> I don't believe so. The violation state function can run with a >> >>>> non-NULL association >> >>>> if we are encountering protocol violations after the association is >> >>>> established. >> >>> >> >>> >> >>> Yup, that's correct. I just tried to reference one case on which it >> >>> would return _ABORT without issuing any of those _FAILEDs before doing >> >>> so (meaning the association could still be valid) but that in that case, >> >>> the asoc was already NULL. >> >> >> >> >> >> I think it is possible to hit the 'discard:' tag in that function while >> >> still >> >> having a valid association. That happens when ABORT chunk is required to >> >> be >> >> authenticated. This that case, instead of generating an ABORT and >> >> terminating the >> >> current association, we just drop the packet, but still report an _ABORT >> >> disposition code. >> >> >> >> This probably need to change if we are going to catch the _ABORT >> >> disposition and >> >> clear the asoc pointer. >> > >> > >> > Oups. Nice one. I'll switch it to SCTP_DISPOSITION_DISCARD if it hits that >> > if() then. Thanks Vlad. >> >> >> So I am waiting for a new patch, right? >> Can you please combine all changes into a single patch (as far as I >> understand the previous one must be applied on top of the first one)? > > The patches were combined already, but this last pick by Vlad is just > not yet patched. It's not necessary for your testing and I didn't want > to interrupt it in case you were already testing it. > > You can use my last patch here, from 2 emails ago, the one which > contains this line: > - case SCTP_DISPOSITION_ABORT: You are right. I missed that they are combined. Testing with it now. -- To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
