iked(8): fix error handling in msg_send

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

iked(8): fix error handling in msg_send

Tobias Heider-2
Hi,

in the error case ikev2_msg_send the accesses the sa before checking for
NULL. The diff adds explicit checks in those cases.
If sendtofrom fails for any other reason than EADDRNOTAVAIL and sa is not NULL
we should continue instead of returning (-1) so that the error is handled with
retransmission.

ok?

Index: ikev2_msg.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2_msg.c,v
retrieving revision 1.58
diff -u -p -r1.58 ikev2_msg.c
--- ikev2_msg.c 13 Nov 2019 12:24:40 -0000 1.58
+++ ikev2_msg.c 14 Nov 2019 15:37:11 -0000
@@ -339,15 +339,20 @@ ikev2_msg_send(struct iked *env, struct
     (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen,
     (struct sockaddr *)&msg->msg_local, msg->msg_locallen) == -1) {
  if (errno == EADDRNOTAVAIL) {
- sa_state(env, msg->msg_sa, IKEV2_STATE_CLOSING);
- timer_del(env, &msg->msg_sa->sa_timer);
- timer_set(env, &msg->msg_sa->sa_timer,
-    ikev2_ike_sa_timeout, msg->msg_sa);
- timer_add(env, &msg->msg_sa->sa_timer,
-    IKED_IKE_SA_DELETE_TIMEOUT);
+ if (sa != NULL) {
+ sa_state(env, sa, IKEV2_STATE_CLOSING);
+ timer_del(env, &sa->sa_timer);
+ timer_set(env, &sa->sa_timer,
+    ikev2_ike_sa_timeout, sa);
+ timer_add(env, &sa->sa_timer,
+    IKED_IKE_SA_DELETE_TIMEOUT);
+ }
+ log_warn("%s: sendtofrom", __func__);
+ return (-1);
  }
  log_warn("%s: sendtofrom", __func__);
- return (-1);
+ if (!sa)
+ return (-1);
  }
 
  if (!sa)

Reply | Threaded
Open this post in threaded view
|

Re: iked(8): fix error handling in msg_send

Mike Belopuhov-5

Tobias Heider writes:

> Hi,
>
> in the error case ikev2_msg_send the accesses the sa before checking for
> NULL. The diff adds explicit checks in those cases.
> If sendtofrom fails for any other reason than EADDRNOTAVAIL and sa is not NULL
> we should continue instead of returning (-1) so that the error is handled with
> retransmission.
>
> ok?
>

Hi Tobias,

you can write a simpler diff w/o repeating log_warn:

diff --git a/sbin/iked/ikev2_msg.c b/sbin/iked/ikev2_msg.c
index 2baea5f5508..396fea88c16 100644
--- a/sbin/iked/ikev2_msg.c
+++ b/sbin/iked/ikev2_msg.c
@@ -338,7 +338,8 @@ ikev2_msg_send(struct iked *env, struct iked_message *msg)
  if (sendtofrom(msg->msg_fd, ibuf_data(buf), ibuf_size(buf), 0,
     (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen,
     (struct sockaddr *)&msg->msg_local, msg->msg_locallen) == -1) {
- if (errno == EADDRNOTAVAIL) {
+ log_warn("%s: sendtofrom", __func__);
+ if (msg->msg_sa != NULL && errno == EADDRNOTAVAIL) {
  sa_state(env, msg->msg_sa, IKEV2_STATE_CLOSING);
  timer_del(env, &msg->msg_sa->sa_timer);
  timer_set(env, &msg->msg_sa->sa_timer,
@@ -346,8 +347,8 @@ ikev2_msg_send(struct iked *env, struct iked_message *msg)
  timer_add(env, &msg->msg_sa->sa_timer,
     IKED_IKE_SA_DELETE_TIMEOUT);
  }
- log_warn("%s: sendtofrom", __func__);
- return (-1);
+ if (msg->msg_sa != NULL)
+ return (-1);
  }
 
  if (!sa)


Regards,
Mike


> Index: ikev2_msg.c
> ===================================================================
> RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2_msg.c,v
> retrieving revision 1.58
> diff -u -p -r1.58 ikev2_msg.c
> --- ikev2_msg.c 13 Nov 2019 12:24:40 -0000 1.58
> +++ ikev2_msg.c 14 Nov 2019 15:37:11 -0000
> @@ -339,15 +339,20 @@ ikev2_msg_send(struct iked *env, struct
>      (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen,
>      (struct sockaddr *)&msg->msg_local, msg->msg_locallen) == -1) {
>   if (errno == EADDRNOTAVAIL) {
> - sa_state(env, msg->msg_sa, IKEV2_STATE_CLOSING);
> - timer_del(env, &msg->msg_sa->sa_timer);
> - timer_set(env, &msg->msg_sa->sa_timer,
> -    ikev2_ike_sa_timeout, msg->msg_sa);
> - timer_add(env, &msg->msg_sa->sa_timer,
> -    IKED_IKE_SA_DELETE_TIMEOUT);
> + if (sa != NULL) {
> + sa_state(env, sa, IKEV2_STATE_CLOSING);
> + timer_del(env, &sa->sa_timer);
> + timer_set(env, &sa->sa_timer,
> +    ikev2_ike_sa_timeout, sa);
> + timer_add(env, &sa->sa_timer,
> +    IKED_IKE_SA_DELETE_TIMEOUT);
> + }
> + log_warn("%s: sendtofrom", __func__);
> + return (-1);
>   }
>   log_warn("%s: sendtofrom", __func__);
> - return (-1);
> + if (!sa)
> + return (-1);
>   }
>  
>   if (!sa)

Reply | Threaded
Open this post in threaded view
|

Re: iked(8): fix error handling in msg_send

Tobias Heider-2
On Thu, Nov 14, 2019 at 05:25:48PM +0100, Mike Belopuhov wrote:

>
> Tobias Heider writes:
>
> > Hi,
> >
> > in the error case ikev2_msg_send the accesses the sa before checking for
> > NULL. The diff adds explicit checks in those cases.
> > If sendtofrom fails for any other reason than EADDRNOTAVAIL and sa is not NULL
> > we should continue instead of returning (-1) so that the error is handled with
> > retransmission.
> >
> > ok?
> >
>
> Hi Tobias,
>
> you can write a simpler diff w/o repeating log_warn:
>
> diff --git a/sbin/iked/ikev2_msg.c b/sbin/iked/ikev2_msg.c
> index 2baea5f5508..396fea88c16 100644
> --- a/sbin/iked/ikev2_msg.c
> +++ b/sbin/iked/ikev2_msg.c
> @@ -338,7 +338,8 @@ ikev2_msg_send(struct iked *env, struct iked_message *msg)
>   if (sendtofrom(msg->msg_fd, ibuf_data(buf), ibuf_size(buf), 0,
>      (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen,
>      (struct sockaddr *)&msg->msg_local, msg->msg_locallen) == -1) {
> - if (errno == EADDRNOTAVAIL) {
> + log_warn("%s: sendtofrom", __func__);
> + if (msg->msg_sa != NULL && errno == EADDRNOTAVAIL) {
>   sa_state(env, msg->msg_sa, IKEV2_STATE_CLOSING);
>   timer_del(env, &msg->msg_sa->sa_timer);
>   timer_set(env, &msg->msg_sa->sa_timer,
> @@ -346,8 +347,8 @@ ikev2_msg_send(struct iked *env, struct iked_message *msg)
>   timer_add(env, &msg->msg_sa->sa_timer,
>      IKED_IKE_SA_DELETE_TIMEOUT);
>   }
> - log_warn("%s: sendtofrom", __func__);
> - return (-1);
> + if (msg->msg_sa != NULL)
> + return (-1);
>   }
>  
>   if (!sa)
>
>
> Regards,
> Mike

The problem here is that log_warn can change errno, if you want to do it this
way you will have to save errno and check against the saved variable.
If you want to stick with msg_sa it would probably make sense to also remove
sa and use msg_sa everywhere. Otherwise I'm ok with yours.

Regards,
Tobias

Reply | Threaded
Open this post in threaded view
|

Re: iked(8): fix error handling in msg_send

Theo de Raadt-2
Tobias Heider <[hidden email]> wrote:

> On Thu, Nov 14, 2019 at 05:25:48PM +0100, Mike Belopuhov wrote:
> >
> > Tobias Heider writes:
> >
> > > Hi,
> > >
> > > in the error case ikev2_msg_send the accesses the sa before checking for
> > > NULL. The diff adds explicit checks in those cases.
> > > If sendtofrom fails for any other reason than EADDRNOTAVAIL and sa is not NULL
> > > we should continue instead of returning (-1) so that the error is handled with
> > > retransmission.
> > >
> > > ok?
> > >
> >
> > Hi Tobias,
> >
> > you can write a simpler diff w/o repeating log_warn:
> >
> > diff --git a/sbin/iked/ikev2_msg.c b/sbin/iked/ikev2_msg.c
> > index 2baea5f5508..396fea88c16 100644
> > --- a/sbin/iked/ikev2_msg.c
> > +++ b/sbin/iked/ikev2_msg.c
> > @@ -338,7 +338,8 @@ ikev2_msg_send(struct iked *env, struct iked_message *msg)
> >   if (sendtofrom(msg->msg_fd, ibuf_data(buf), ibuf_size(buf), 0,
> >      (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen,
> >      (struct sockaddr *)&msg->msg_local, msg->msg_locallen) == -1) {
> > - if (errno == EADDRNOTAVAIL) {
> > + log_warn("%s: sendtofrom", __func__);
> > + if (msg->msg_sa != NULL && errno == EADDRNOTAVAIL) {
> >   sa_state(env, msg->msg_sa, IKEV2_STATE_CLOSING);
> >   timer_del(env, &msg->msg_sa->sa_timer);
> >   timer_set(env, &msg->msg_sa->sa_timer,
> > @@ -346,8 +347,8 @@ ikev2_msg_send(struct iked *env, struct iked_message *msg)
> >   timer_add(env, &msg->msg_sa->sa_timer,
> >      IKED_IKE_SA_DELETE_TIMEOUT);
> >   }
> > - log_warn("%s: sendtofrom", __func__);
> > - return (-1);
> > + if (msg->msg_sa != NULL)
> > + return (-1);
> >   }
> >  
> >   if (!sa)
> >
> >
> > Regards,
> > Mike
>
> The problem here is that log_warn can change errno,

No, it specifically avoids touching errno.

log_warn(const char *emsg, ...)
{
        char            *nfmt;
        va_list          ap;
        int              saved_errno = errno;
...
        errno = saved_errno;
}

Reply | Threaded
Open this post in threaded view
|

Re: iked(8): fix error handling in msg_send

Tobias Heider-2
On Thu, Nov 14, 2019 at 09:57:27AM -0700, Theo de Raadt wrote:

> >
> > The problem here is that log_warn can change errno,
>
> No, it specifically avoids touching errno.
>
> log_warn(const char *emsg, ...)
> {
>         char            *nfmt;
>         va_list          ap;
>         int              saved_errno = errno;
> ...
>         errno = saved_errno;
> }
>

Good to know, thanks! In that case I really prefer Mike's diff.
Here is an update with msg->msg_sa used consistently. We can also do it
the other way around, but I would prefer to use either sa or msg_sa.

Index: ikev2_msg.c
===================================================================
RCS file: /cvs/src/sbin/iked/ikev2_msg.c,v
retrieving revision 1.58
diff -u -p -r1.58 ikev2_msg.c
--- ikev2_msg.c 13 Nov 2019 12:24:40 -0000 1.58
+++ ikev2_msg.c 14 Nov 2019 17:15:42 -0000
@@ -303,7 +303,6 @@ ikev2_msg_valid_ike_sa(struct iked *env,
 int
 ikev2_msg_send(struct iked *env, struct iked_message *msg)
 {
- struct iked_sa *sa = msg->msg_sa;
  struct ibuf *buf = msg->msg_data;
  uint32_t natt = 0x00000000;
  int isnatt = 0;
@@ -338,7 +337,8 @@ ikev2_msg_send(struct iked *env, struct
  if (sendtofrom(msg->msg_fd, ibuf_data(buf), ibuf_size(buf), 0,
     (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen,
     (struct sockaddr *)&msg->msg_local, msg->msg_locallen) == -1) {
- if (errno == EADDRNOTAVAIL) {
+ log_warn("%s: sendtofrom", __func__);
+ if (msg->msg_sa != NULL && errno == EADDRNOTAVAIL) {
  sa_state(env, msg->msg_sa, IKEV2_STATE_CLOSING);
  timer_del(env, &msg->msg_sa->sa_timer);
  timer_set(env, &msg->msg_sa->sa_timer,
@@ -346,11 +346,11 @@ ikev2_msg_send(struct iked *env, struct
  timer_add(env, &msg->msg_sa->sa_timer,
     IKED_IKE_SA_DELETE_TIMEOUT);
  }
- log_warn("%s: sendtofrom", __func__);
- return (-1);
+ if (msg->msg_sa != NULL)
+ return (-1);
  }
 
- if (!sa)
+ if (msg->msg_sa == NULL)
  return (0);
 
  if ((m = ikev2_msg_copy(env, msg)) == NULL) {
@@ -360,11 +360,11 @@ ikev2_msg_send(struct iked *env, struct
  m->msg_exchange = exchange;
 
  if (flags & IKEV2_FLAG_RESPONSE) {
- TAILQ_INSERT_TAIL(&sa->sa_responses, m, msg_entry);
+ TAILQ_INSERT_TAIL(&msg->msg_sa->sa_responses, m, msg_entry);
  timer_set(env, &m->msg_timer, ikev2_msg_response_timeout, m);
  timer_add(env, &m->msg_timer, IKED_RESPONSE_TIMEOUT);
  } else {
- TAILQ_INSERT_TAIL(&sa->sa_requests, m, msg_entry);
+ TAILQ_INSERT_TAIL(&msg->msg_sa->sa_requests, m, msg_entry);
  timer_set(env, &m->msg_timer, ikev2_msg_retransmit_timeout, m);
  timer_add(env, &m->msg_timer, IKED_RETRANSMIT_TIMEOUT);
  }

Reply | Threaded
Open this post in threaded view
|

Re: iked(8): fix error handling in msg_send

Mike Belopuhov-5

Tobias Heider writes:

> On Thu, Nov 14, 2019 at 09:57:27AM -0700, Theo de Raadt wrote:
>> >
>> > The problem here is that log_warn can change errno,
>>
>> No, it specifically avoids touching errno.
>>
>> log_warn(const char *emsg, ...)
>> {
>>         char            *nfmt;
>>         va_list          ap;
>>         int              saved_errno = errno;
>> ...
>>         errno = saved_errno;
>> }
>>
>
> Good to know, thanks! In that case I really prefer Mike's diff.
> Here is an update with msg->msg_sa used consistently. We can also do it
> the other way around, but I would prefer to use either sa or msg_sa.

I'm sorry, it was a bit irresponsible of me to put msg_sa everywhere
in my diff.  In fact, almost all of the code in this file uses 'sa'
so it would be consistent with other functions if 'sa' would be used
instead of 'msg_sa'.  If you don't mind changing it to 'sa', I would
appreciate it.  OK mikeb either way.

>
> Index: ikev2_msg.c
> ===================================================================
> RCS file: /cvs/src/sbin/iked/ikev2_msg.c,v
> retrieving revision 1.58
> diff -u -p -r1.58 ikev2_msg.c
> --- ikev2_msg.c 13 Nov 2019 12:24:40 -0000 1.58
> +++ ikev2_msg.c 14 Nov 2019 17:15:42 -0000
> @@ -303,7 +303,6 @@ ikev2_msg_valid_ike_sa(struct iked *env,
>  int
>  ikev2_msg_send(struct iked *env, struct iked_message *msg)
>  {
> - struct iked_sa *sa = msg->msg_sa;
>   struct ibuf *buf = msg->msg_data;
>   uint32_t natt = 0x00000000;
>   int isnatt = 0;
> @@ -338,7 +337,8 @@ ikev2_msg_send(struct iked *env, struct
>   if (sendtofrom(msg->msg_fd, ibuf_data(buf), ibuf_size(buf), 0,
>      (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen,
>      (struct sockaddr *)&msg->msg_local, msg->msg_locallen) == -1) {
> - if (errno == EADDRNOTAVAIL) {
> + log_warn("%s: sendtofrom", __func__);
> + if (msg->msg_sa != NULL && errno == EADDRNOTAVAIL) {
>   sa_state(env, msg->msg_sa, IKEV2_STATE_CLOSING);
>   timer_del(env, &msg->msg_sa->sa_timer);
>   timer_set(env, &msg->msg_sa->sa_timer,
> @@ -346,11 +346,11 @@ ikev2_msg_send(struct iked *env, struct
>   timer_add(env, &msg->msg_sa->sa_timer,
>      IKED_IKE_SA_DELETE_TIMEOUT);
>   }
> - log_warn("%s: sendtofrom", __func__);
> - return (-1);
> + if (msg->msg_sa != NULL)
> + return (-1);
>   }
>  
> - if (!sa)
> + if (msg->msg_sa == NULL)
>   return (0);
>  
>   if ((m = ikev2_msg_copy(env, msg)) == NULL) {
> @@ -360,11 +360,11 @@ ikev2_msg_send(struct iked *env, struct
>   m->msg_exchange = exchange;
>  
>   if (flags & IKEV2_FLAG_RESPONSE) {
> - TAILQ_INSERT_TAIL(&sa->sa_responses, m, msg_entry);
> + TAILQ_INSERT_TAIL(&msg->msg_sa->sa_responses, m, msg_entry);
>   timer_set(env, &m->msg_timer, ikev2_msg_response_timeout, m);
>   timer_add(env, &m->msg_timer, IKED_RESPONSE_TIMEOUT);
>   } else {
> - TAILQ_INSERT_TAIL(&sa->sa_requests, m, msg_entry);
> + TAILQ_INSERT_TAIL(&msg->msg_sa->sa_requests, m, msg_entry);
>   timer_set(env, &m->msg_timer, ikev2_msg_retransmit_timeout, m);
>   timer_add(env, &m->msg_timer, IKED_RETRANSMIT_TIMEOUT);
>   }

Reply | Threaded
Open this post in threaded view
|

Re: iked(8): fix error handling in msg_send

Tobias Heider-2
> I'm sorry, it was a bit irresponsible of me to put msg_sa everywhere
> in my diff.  In fact, almost all of the code in this file uses 'sa'
> so it would be consistent with other functions if 'sa' would be used
> instead of 'msg_sa'.  If you don't mind changing it to 'sa', I would
> appreciate it.  OK mikeb either way.

Thanks, committed with 'sa'!