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: Marcelo -- 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
