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)? -- 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
