Re: [lwip-devel] bug in calling tcp_output_fill_options() ?

[Hannes Gredler]

hi simon,

thanks for the quick review - just figured that i was taking the patch from the 
wrong tree;
correct one is below and should apply cleanly;

/hannes

diff --git a/src/core/tcp_out.c b/src/core/tcp_out.c
index 7d7ea5ee..bfb033b1 100644
--- a/src/core/tcp_out.c
+++ b/src/core/tcp_out.c
@@ -2002,7 +2002,7 @@ tcp_rst(const struct tcp_pcb *pcb, u32_t seqno, u32_t 
ackno,
     LWIP_DEBUGF(TCP_DEBUG, ("tcp_rst: could not allocate memory for pbuf\n"));
     return;
   }
-  tcp_output_fill_options(pcb, p, 0, optlen);
+  tcp_output_fill_options(pcb, p, 0, 0);

   MIB2_STATS_INC(mib2.tcpoutrsts);

@@ -2096,7 +2096,7 @@ tcp_keepalive(struct tcp_pcb *pcb)
                 ("tcp_keepalive: could not allocate memory for pbuf\n"));
     return ERR_MEM;
   }
-  tcp_output_fill_options(pcb, p, 0, optlen);
+  tcp_output_fill_options(pcb, p, 0, 0);
   err = tcp_output_control_segment(pcb, p, &pcb->local_ip, &pcb->remote_ip);

   LWIP_DEBUGF(TCP_DEBUG, ("tcp_keepalive: seqno %"U32_F" ackno %"U32_F" err 
%d.\n",
@@ -2178,7 +2178,7 @@ tcp_zero_window_probe(struct tcp_pcb *pcb)
   if (TCP_SEQ_LT(pcb->snd_nxt, snd_nxt)) {
     pcb->snd_nxt = snd_nxt;
   }
-  tcp_output_fill_options(pcb, p, 0, optlen);
+  tcp_output_fill_options(pcb, p, 0, 0);

   err = tcp_output_control_segment(pcb, p, &pcb->local_ip, &pcb->remote_ip);

On Wed, Apr 01, 2020 at 11:19:27AM +0200, address@hidden wrote:
| Am 01.04.2020 um 11:05 schrieb Hannes Gredler:
| > hi,
| >
| > I have run into a bug in tcp_output_fill_options() while trying to add 
TCP-AO (RFC5925) support.
| >
| > --
| >
| > it is my understanding that fourth argument in the callstack is the number 
of SACKs.
| >
| > static void
| > tcp_output_fill_options(const struct tcp_pcb *pcb, struct pbuf *p, u8_t 
optflags, u8_t num_sacks)
| >                                                                             
      ^^^^^^^^^^^^^^^
| >
| > however there is still 4 calls which pass in the total option length which 
gets then
| > misinterpreted as number of SACKs. which in my case caused a buffer 
overflow in
| > tcp_output_fill_options()
| >
| > Q: shouldn't all those arguments reset to zero as per below patch ?
| 
| Seems like you're right, yes.
| 
| Regards,
| Simon
| 
| >
| > thanks,
| >
| > /hannes
| >
| >
| > diff --git a/src/core/tcp_out.c b/src/core/tcp_out.c
| > index 4e7cbb09..63c37448 100644
| > --- a/src/core/tcp_out.c
| > +++ b/src/core/tcp_out.c
| > @@ -2034,7 +2034,7 @@ tcp_rst(const struct tcp_pcb *pcb, u32_t seqno, u32_t 
ackno,
| >      LWIP_DEBUGF(TCP_DEBUG, ("tcp_rst: could not allocate memory for 
pbuf\n"));
| >      return;
| >    }
| > -  tcp_output_fill_options(pcb, p, 0, optlen);
| > +  tcp_output_fill_options(pcb, p, 0, 0);
| >
| >  #ifdef LWIP_CUSTOM_HOOK_TCP_OUT_ADD
| >      LWIP_CUSTOM_HOOK_TCP_OUT_ADD(p, pcb);
| > @@ -2132,7 +2132,7 @@ tcp_keepalive(struct tcp_pcb *pcb)
| >                  ("tcp_keepalive: could not allocate memory for pbuf\n"));
| >      return ERR_MEM;
| >    }
| > -  tcp_output_fill_options(pcb, p, 0, optlen);
| > +  tcp_output_fill_options(pcb, p, 0, 0);
| >    err = tcp_output_control_segment(pcb, p, &pcb->local_ip, 
&pcb->remote_ip);
| >
| >    LWIP_DEBUGF(TCP_DEBUG, ("tcp_keepalive: seqno %"U32_F" ackno %"U32_F" 
err %d.\n",
| > @@ -2214,7 +2214,7 @@ tcp_zero_window_probe(struct tcp_pcb *pcb)
| >    if (TCP_SEQ_LT(pcb->snd_nxt, snd_nxt)) {
| >      pcb->snd_nxt = snd_nxt;
| >    }
| > -  tcp_output_fill_options(pcb, p, 0, optlen);
| > +  tcp_output_fill_options(pcb, p, 0, 0);
| >
| >    err = tcp_output_control_segment(pcb, p, &pcb->local_ip, 
&pcb->remote_ip);
| >
| > @@ -2259,7 +2259,7 @@ tcp_custom_rst(s8_t *instName, u32_t seqno, u32_t 
ackno,
| >      LWIP_DEBUGF(TCP_DEBUG, ("tcp_rst: could not allocate memory for 
pbuf\n"));
| >      return;
| >    }
| > -  tcp_output_fill_options(pcb, p, 0, optlen);
| > +  tcp_output_fill_options(pcb, p, 0, 0);
| >
| >    p->instName = instName;
| >
| > _______________________________________________
| > lwip-devel mailing list
| > address@hidden
| > https://lists.nongnu.org/mailman/listinfo/lwip-devel
| >
| 
| 
| _______________________________________________
| lwip-devel mailing list
| address@hidden
| https://lists.nongnu.org/mailman/listinfo/lwip-devel