[PATCH] Patchset for bug #10344 - SessionLogoff on a signed connection with an outstanding notify request crashes smbd.
Jeremy Allison
jra at samba.org
Tue Mar 4 10:06:26 MST 2014
On Tue, Mar 04, 2014 at 12:30:52PM +0100, Stefan (metze) Metzmacher wrote:
> Hi Jeremy,
>
> I really hate this patches, but I don't have something better currently :-)
Why thank you :-).
> here my comments regarding the session case, most of them also apply to the
> tcon case.
>
> Please also go to the patchset and check if the comments are nicely
> formated.
>
> More comments below.
Thanks for the review, I'll fix them up and re-send.
Cheers,
Jeremy.
> > Add smbd_smb2_logoff_send()/smbd_smb2_logoff_recv()/smbd_smb2_request_logoff_done()
> > and make smbd_smb2_logoff_send() re-entrant. First call to smbd_smb2_logoff_send()
> > calls tevent_req_cancel() on any outstanding requests on this session
> > then re-schedules itself to be processed after the canceled requests
> > finish. It waits 30 seconds, re-scheduling itself until all events
> > have been canceled.
> >
> > [Bug 10344] SessionLogoff on a signed connection with an outstanding notify request crashes smbd.
> >
> > https://bugzilla.samba.org/show_bug.cgi?id=10344
> >
> > Signed-off-by: Jeremy Allison <jra at samba.org>
> > ---
> > source3/smbd/smb2_sesssetup.c | 178 +++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 158 insertions(+), 20 deletions(-)
> >
> > diff --git a/source3/smbd/smb2_sesssetup.c b/source3/smbd/smb2_sesssetup.c
> > index 7dbf603..3763a0c 100644
> > --- a/source3/smbd/smb2_sesssetup.c
> > +++ b/source3/smbd/smb2_sesssetup.c
> > @@ -880,44 +880,182 @@ static bool cancel_outstanding_session_requests(const struct smbd_smb2_request *
> > return ret;
> > }
> >
> > +static struct tevent_req *smbd_smb2_logoff_send(TALLOC_CTX *mem_ctx,
> > + struct tevent_context *ev,
> > + struct smbd_smb2_request *smb2req);
> > +static NTSTATUS smbd_smb2_logoff_recv(struct tevent_req *req);
> > +static void smbd_smb2_request_logoff_done(struct tevent_req *subreq);
> > +
> > NTSTATUS smbd_smb2_request_process_logoff(struct smbd_smb2_request *req)
> > {
> > NTSTATUS status;
> > - DATA_BLOB outbody;
> > + struct tevent_req *subreq = NULL;
> >
> > status = smbd_smb2_request_verify_sizes(req, 0x04);
> > if (!NT_STATUS_IS_OK(status)) {
> > return smbd_smb2_request_error(req, status);
> > }
> >
> > + subreq = smbd_smb2_logoff_send(req, req->sconn->ev_ctx, req);
> > + if (subreq == NULL) {
> > + return smbd_smb2_request_error(req, NT_STATUS_NO_MEMORY);
> > + }
> > + tevent_req_set_callback(subreq, smbd_smb2_request_logoff_done, req);
> > + /* Ensure we don't reply with an async message on this call -
> > + set timeout to 120 secs. */
> > + return smbd_smb2_request_pending_queue(req, subreq, 120000000);
> > +}
> > +
> > +static void smbd_smb2_request_logoff_done(struct tevent_req *subreq)
> > +{
> > + struct smbd_smb2_request *smb2req =
> > + tevent_req_callback_data(subreq,
> > + struct smbd_smb2_request);
> > + DATA_BLOB outbody;
> > + NTSTATUS status;
> > + NTSTATUS error;
> > +
> > + status = smbd_smb2_logoff_recv(subreq);
> > + TALLOC_FREE(subreq);
> > + if (!NT_STATUS_IS_OK(status)) {
> > + error = smbd_smb2_request_error(smb2req, status);
> > + if (!NT_STATUS_IS_OK(error)) {
> > + smbd_server_connection_terminate(smb2req->sconn,
> > + nt_errstr(error));
> > + return;
> > + }
> > + return;
> > + }
> > +
> > + outbody = data_blob_talloc(smb2req->out.vector, NULL, 0x04);
> > + if (outbody.data == NULL) {
> > + error = smbd_smb2_request_error(smb2req, NT_STATUS_NO_MEMORY);
> > + if (!NT_STATUS_IS_OK(error)) {
> > + smbd_server_connection_terminate(smb2req->sconn,
> > + nt_errstr(error));
> > + return;
> > + }
> > + return;
> > + }
> > +
> > + SSVAL(outbody.data, 0x00, 0x04); /* struct size */
> > + SSVAL(outbody.data, 0x02, 0); /* reserved */
> > +
> > + error = smbd_smb2_request_done(smb2req, outbody, NULL);
> > + if (!NT_STATUS_IS_OK(error)) {
> > + smbd_server_connection_terminate(smb2req->sconn,
> > + nt_errstr(error));
> > + return;
> > + }
> > +}
> > +
> > +struct smbd_smb2_logout_state {
> > + struct already_cancelled *acp;
> > + struct timeval end_logout;
> > +};
> > +
> > +static struct tevent_req *smbd_smb2_logoff_send(TALLOC_CTX *mem_ctx,
> > + struct tevent_context *ev,
> > + struct smbd_smb2_request *smb2req)
> > +{
> > + NTSTATUS status;
> > + struct tevent_req *req;
> > + struct smbd_smb2_logout_state *state;
> > +
> > + req = smb2req->subreq;
> > + if (req == NULL) {
> > + /* New logoff call. */
> > + req = tevent_req_create(mem_ctx, &state,
> > + struct smbd_smb2_logout_state);
> > + if (req == NULL) {
> > + return NULL;
>
> This should be the only case where we should return NULL.
>
> > + }
> > +
> > + state->acp = talloc_zero(state, struct already_cancelled);
> > + if (state->acp == NULL) {
>
> please use tevent_req_nomem() and tevent_req_post().
>
> > + return NULL;
> > + }
> > +
> > + /* Don't wait more than 30 seconds for cancel.
> > + So remember time now + 30 seconds. If we are
> > + re-scheduled after this the cancel requests
> > + took too long and we exit. */
>
> Please see README.Coding and reformat the comment.
>
> > + state->end_logout = tevent_timeval_current_ofs(30, 0);
> > + } else {
> > + /* Re-entrant logoff call - we are being rescheduled
> > + after we're requested other requests are canceled. */
> > + struct timeval curr = tevent_timeval_current();
> > + state = tevent_req_data(req, struct smbd_smb2_logout_state);
> > +
> > + if (tevent_timeval_compare(&curr, &state->end_logout) >= 0) {
> > +
> > + /* Ran out of time - terminate. */
> > + smbd_server_connection_terminate(smb2req->sconn,
> > + nt_errstr(NT_STATUS_DRIVER_CANCEL_TIMEOUT));
> > + return NULL;
>
> Please let smbd_smb2_logoff_recv() return an indication that the caller
> should
> terminate the connection. Maybe bool *is_error.
> This is the wrong place.
>
> I'm not sure maybe any non NT_STATUS_OK return from smbd_smb2_logoff_recv()
> should terminate the connection?
>
> > + }
> > +
> > + /* We're re-entrant. We must clear the callback
> > + data in case we finish here with tevent_done.
> > + If we re-schedule, caller will reset callback
> > + correctly. */
>
> Please see README.Coding and reformat the comment.
>
> > + tevent_req_set_callback(req, NULL, NULL);
>
> I really hate this :-) Please make it more clear that this is a really
> ugly hack
> and nobody should ever learn from this example :-)
>
> > + }
> > +
> > /*
> > - * TODO: cancel all outstanding requests on the session
> > + * Try and cancel a request. If we succeed, reschedule ourselves.
> > */
> > - status = smbXsrv_session_logoff(req->session);
> > - if (!NT_STATUS_IS_OK(status)) {
> > - DEBUG(0, ("smbd_smb2_request_process_logoff: "
> > - "smbXsrv_session_logoff() failed: %s\n",
> > - nt_errstr(status)));
> > - /*
> > - * If we hit this case, there is something completely
> > - * wrong, so we better disconnect the transport connection.
> > - */
> > - return status;
> > + if (cancel_outstanding_session_requests(smb2req, state->acp)) {
> > + struct tevent_immediate *im = tevent_create_immediate(smb2req);
> > + if (!im) {
>
> Please use tevent_req_nomem() and tevent_req_post()
>
> > + return NULL;
> > + }
>
> Shouldn't we mark the session as deleted here?
> New requests can still use the session and will trigger the same crash
> as before.
>
> > + /* Try again later. */
> > + tevent_schedule_immediate(im,
> > + smb2req->sconn->ev_ctx,
> > + smbd_smb2_request_dispatch_immediate,
> > + smb2req);
>
> Doesn't this result in a 100% CPU loop? How will any really pending request
> e.g. async pread, ever get scheduled?
>
> > + return req;
> > + }
> > +
> > + /*
>
> Please fix the indentation here.
>
> > + * When we get here, we are actually going to tear
> > + * down the sesssion pointer.
> > + */
> > + status = smbXsrv_session_logoff(smb2req->session);
> > + if (tevent_req_nterror(req, status)) {
> > + DEBUG(0, ("smbXsrv_session_logoff() failed: %s\n",
> > + nt_errstr(status)));
> > + return tevent_req_post(req, ev);
> > }
> >
> > /*
> > * we may need to sign the response, so we need to keep
> > * the session until the response is sent to the wire.
> > */
> > - talloc_steal(req, req->session);
> > + talloc_steal(smb2req, smb2req->session);
> >
> > - outbody = data_blob_talloc(req->out.vector, NULL, 0x04);
> > - if (outbody.data == NULL) {
> > - return smbd_smb2_request_error(req, NT_STATUS_NO_MEMORY);
> > - }
> > + /*
> > + * The return from smbXsrv_session_logoff()
> > + * leaves all outstanding pointers to
> > + * smb2req->session not on this request in an
> > + * undefined state, ensure we don't try and
> > + * access any req->session pointers once
> > + * we return.
> > + */
> > + remove_outstanding_session_refs(smb2req);
> >
> > - SSVAL(outbody.data, 0x00, 0x04); /* struct size */
> > - SSVAL(outbody.data, 0x02, 0); /* reserved */
> > + tevent_req_done(req);
> > + return tevent_req_post(req, ev);
> > +}
> > +
> > +static NTSTATUS smbd_smb2_logoff_recv(struct tevent_req *req)
> > +{
> > + NTSTATUS status;
> >
> > - return smbd_smb2_request_done(req, outbody, NULL);
> > + if (tevent_req_is_nterror(req, &status)) {
> > + tevent_req_received(req);
> > + return status;
> > + }
> > + tevent_req_received(req);
> > + return NT_STATUS_OK;
> > }
More information about the samba-technical
mailing list