max_start_pad: calculate globally, not via client graph

There is quite a lot of plumbing of max_start_pad values from one
place to another.  Sadly this plumbing is not complete, which can lead
to crashes as the start padding is exhausted.  And completing it is a
lot of work which would be difficult to get correct, even if that's
possible in principle.

Instead, we take a different approach.  We calculate a single global
max_pad_start value that can be used everywhere.  It is the sum of:

 * Headers that "site" instances might need add to the start of
   packets (source and destination site indices, message type both
   inside and outside the encryption;

 * Anything that "transform" instances might need to add to the start
   of packets.  This depends on the transforms, but since it isn't a
   priori predictable which path any particular incoming packet might
   take, we have to take the worst case.

   These transform instances are applied only by "site" and each
   packet goes through at most on "forward" transform instance.

 * Anything that "comm" instances might need to add.  This is
   currently only needed for the proxy feature.

This is based on the assumption that a packet may follow one of these
paths:

    comm -->- site_incoming --.         ,-- site_outgoing -->- comm
                               \       /
                                netlink
                               /       \
    tun/slip ----->-----------'         `------>---------- tun/slip

On the inbound side, tun and slip have to set up the buffer with the
necessary start padding.

site_incoming only removes stuff from the beginning (since it uses
transform->reverse).  netlink doesn't add or remove anything.

It is site_outgoing and comm which may need to prepend things.  site
additionally calls transform->forwards.

If in the future a module appears which can take packets out of the
RHS of this diagram and feed them back into the left, it may have to
do something about the buffer.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

diff --git a/netlink.c b/netlink.c
index 5d586d03dbd14e4e6b9ba92a36db1b5c0ecb50b3..173d412c96c998f0fa15052005b0cedc8e9005a4 100644 (file)
--- a/netlink.c
+++ b/netlink.c
@@ -238,7 +238,7 @@ static struct icmphdr *netlink_icmp_tmpl(struct netlink *st,
     struct icmphdr *h;
 
     BUF_ALLOC(&st->icmp,"netlink_icmp_tmpl");
-    buffer_init(&st->icmp,st->max_start_pad);
+    buffer_init(&st->icmp,calculate_max_start_pad());
     h=buf_append(&st->icmp,sizeof(*h));
 
     h->iph.version=4;
@@ -808,12 +808,10 @@ static void netlink_inst_set_mtu(void *sst, int32_t new_mtu)
 }
 
 static void netlink_inst_reg(void *sst, netlink_deliver_fn *deliver, 
-                            void *dst, int32_t max_start_pad)
+                            void *dst)
 {
     struct netlink_client *c=sst;
-    struct netlink *st=c->nst;
 
-    if (max_start_pad > st->max_start_pad) st->max_start_pad=max_start_pad;
     c->deliver=deliver;
     c->dst=dst;
 }
@@ -941,7 +939,6 @@ netlink_deliver_fn *netlink_init(struct netlink *st,
     st->cl.type=CL_PURE;
     st->cl.apply=netlink_inst_apply;
     st->cl.interface=st;
-    st->max_start_pad=0;
     st->clients=NULL;
     st->routes=NULL;
     st->n_clients=0;

diff --git a/netlink.h b/netlink.h
index 5e6ad49c218de2d22a4ab27a9fc73fe55493ba77..7c427161eae531e5d26238dd97f5544a0b4c8b0f 100644 (file)
--- a/netlink.h
+++ b/netlink.h
@@ -45,7 +45,6 @@ struct netlink {
     closure_t cl;
     void *dst; /* Pointer to host interface state */
     cstring_t name;
-    int32_t max_start_pad;
     struct ipset *networks; /* Local networks */
     struct subnet_list *subnets; /* Same as networks, for display */
     struct ipset *remote_networks; /* Allowable remote networks */

diff --git a/secnet.h b/secnet.h
index 98a3ae38fa548434fcbf3c0a6a35236c43d8e1cc..9bec310c1b8f280a54a0dec91377faad930780f4 100644 (file)
--- a/secnet.h
+++ b/secnet.h
@@ -146,6 +146,16 @@ static const struct timeval *const tv_now = &tv_now_global;
 
 /***** END of utility functions *****/
 
+/***** START of max_start_pad handling *****/
+
+extern int32_t site_max_start_pad, transform_max_start_pad,
+    comm_max_start_pad;
+
+void update_max_start_pad(int32_t *our_module_global, int32_t our_instance);
+int32_t calculate_max_start_pad(void);
+
+/***** END of max_start_pad handling *****/
+
 /***** SCHEDULING support */
 
 /* If nfds_io is insufficient for your needs, set it to the required
@@ -325,7 +335,6 @@ typedef const char *comm_addr_to_string_fn(void *commst,
         /* Returned string is in a static buffer. */
 struct comm_if {
     void *st;
-    int32_t min_start_pad;
     comm_request_notify_fn *request_notify;
     comm_release_notify_fn *release_notify;
     comm_sendmsg_fn *sendmsg;
@@ -363,8 +372,7 @@ struct site_if {
 /* TRANSFORM interface */
 
 /* A reversable transformation. Transforms buffer in-place; may add
-   data to start or end. Maximum amount of data to be added before
-   the packet specified in max_start_pad. (Reverse transformations decrease
+   data to start or end. (Reverse transformations decrease
    length, of course.)  Transformations may be key-dependent, in which
    case key material is passed in at initialisation time. They may
    also depend on internal factors (eg. time) and keep internal
@@ -396,14 +404,12 @@ struct transform_inst_if {
     transform_apply_fn *forwards;
     transform_apply_fn *reverse;
     transform_destroyinstance_fn *destroy;
-    int32_t max_start_pad; /* same as from transform_if */
 };
 
 struct transform_if {
     void *st;
-    int32_t max_start_pad; /* these two are both <<< INT_MAX */
-    int32_t keylen; /* 0 means give the transform exactly as much as there is */
     int capab_transformnum;
+    int32_t keylen; /* <<< INT_MAX */
     transform_createinstance_fn *create;
 };
 
@@ -425,7 +431,7 @@ typedef void netlink_deliver_fn(void *st, struct buffer_if *buf);
 #define MAXIMUM_LINK_QUALITY 3
 typedef void netlink_link_quality_fn(void *st, uint32_t quality);
 typedef void netlink_register_fn(void *st, netlink_deliver_fn *deliver,
-                                void *dst, int32_t max_start_pad);
+                                void *dst);
 typedef void netlink_output_config_fn(void *st, struct buffer_if *buf);
 typedef bool_t netlink_check_config_fn(void *st, struct buffer_if *buf);
 typedef void netlink_set_mtu_fn(void *st, int32_t new_mtu);

diff --git a/site.c b/site.c
index 20f9507cee7782c2f801dba295e9a82421855e88..909e8e9fa5a9903967fc700d93b4c2cabf70a620 100644 (file)
--- a/site.c
+++ b/site.c
@@ -89,6 +89,8 @@
 #define SITE_SENTMSG5 7
 #define SITE_WAIT     8
 
+int32_t site_max_start_pad = 4*4;
+
 static cstring_t state_name(uint32_t state)
 {
     switch (state) {
@@ -826,7 +828,7 @@ static bool_t generate_msg5(struct site *st)
 
     BUF_ALLOC(&st->buffer,"site:MSG5");
     /* We are going to add four words to the message */
-    buffer_init(&st->buffer,st->new_transform->max_start_pad+(4*4));
+    buffer_init(&st->buffer,calculate_max_start_pad());
     /* Give the netlink code an opportunity to put its own stuff in the
        message (configuration information, etc.) */
     buf_prepend_uint32(&st->buffer,LABEL_MSG5);
@@ -873,7 +875,7 @@ static void create_msg6(struct site *st, struct transform_inst_if *transform,
 
     BUF_ALLOC(&st->buffer,"site:MSG6");
     /* We are going to add four words to the message */
-    buffer_init(&st->buffer,transform->max_start_pad+(4*4));
+    buffer_init(&st->buffer,calculate_max_start_pad());
     /* Give the netlink code an opportunity to put its own stuff in the
        message (configuration information, etc.) */
     buf_prepend_uint32(&st->buffer,LABEL_MSG6);
@@ -1294,7 +1296,7 @@ static bool_t send_msg7(struct site *st, cstring_t reason)
     if (current_valid(st) && st->buffer.free
        && transport_peers_valid(&st->peers)) {
        BUF_ALLOC(&st->buffer,"site:MSG7");
-       buffer_init(&st->buffer,st->current.transform->max_start_pad+(4*3));
+       buffer_init(&st->buffer,calculate_max_start_pad());
        buf_append_uint32(&st->buffer,LABEL_MSG7);
        buf_append_string(&st->buffer,reason);
        if (call_transform_forwards(st, st->current.transform,
@@ -1770,17 +1772,6 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context,
     st->sharedsecretlen=st->sharedsecretallocd=0;
     st->sharedsecret=0;
 
-    /* We need to compute some properties of our comms and transports */
-#define COMPUTE_WORST(things,pad)              \
-    int things##_worst_##pad=0;                        \
-    for (i=0; i<st->n##things; i++) {          \
-       int thispad=st->things[i]->pad;         \
-       if (thispad > things##_worst_##pad)     \
-           things##_worst_##pad=thispad;       \
-    }
-    COMPUTE_WORST(comms,min_start_pad)
-    COMPUTE_WORST(transforms,max_start_pad)
-
     for (i=0; i<st->ntransforms; i++) {
        struct transform_if *ti=st->transforms[i];
        uint32_t capbit = 1UL << ti->capab_transformnum;
@@ -1791,9 +1782,7 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context,
     }
 
     /* We need to register the remote networks with the netlink device */
-    st->netlink->reg(st->netlink->st, site_outgoing, st,
-                    transforms_worst_max_start_pad+(4*4)+
-                    comms_worst_min_start_pad);
+    st->netlink->reg(st->netlink->st, site_outgoing, st);
     
     for (i=0; i<st->ncomms; i++)
        st->comms[i]->request_notify(st->comms[i]->st, st, site_incoming);

diff --git a/slip.c b/slip.c
index 0f62a6987268790ce5c4adabf40efa359837a34e..5eb8dbdb1239e72698eb52d0f53d108af2e2f607 100644 (file)
--- a/slip.c
+++ b/slip.c
@@ -115,7 +115,7 @@ static void slip_unstuff(struct slip *st, uint8_t *buf, uint32_t l)
        if (st->ignoring_packet) {
            if (outputchr == OUTPUT_END) {
                st->ignoring_packet=False;
-               buffer_init(st->buff,st->nl.max_start_pad);
+               buffer_init(st->buff,calculate_max_start_pad());
            }
        } else {
            if (outputchr == OUTPUT_END) {
@@ -123,7 +123,7 @@ static void slip_unstuff(struct slip *st, uint8_t *buf, uint32_t l)
                    st->netlink_to_tunnel(&st->nl,st->buff);
                    BUF_ALLOC(st->buff,"userv_afterpoll");
                }
-               buffer_init(st->buff,st->nl.max_start_pad);
+               buffer_init(st->buff,calculate_max_start_pad());
            } else if (outputchr != OUTPUT_NOTHING) {
                if (st->buff->size < st->buff->len) {
                    buf_append_uint8(st->buff,outputchr);

diff --git a/transform-cbcmac.c b/transform-cbcmac.c
index 95c64e859a7abfbaba2839df15e3d7da92fac48b..26e0a127d37cf8dcd7297eab6768795811453629 100644 (file)
--- a/transform-cbcmac.c
+++ b/transform-cbcmac.c
@@ -261,8 +261,8 @@ static list_t *transform_apply(closure_t *self, struct cloc loc,
     st->cl.apply=NULL;
     st->cl.interface=&st->ops;
     st->ops.st=st;
-    st->ops.max_start_pad=28; /* 4byte seqnum, 16byte pad, 4byte MACIV,
-                                4byte IV */
+    update_max_start_pad(&transform_max_start_pad, 28);
+        /* 4byte seqnum, 16byte pad, 4byte MACIV, 4byte IV */
 
     /* We need 256*2 bits for serpent keys, 32 bits for CBC-IV and 32 bits
        for CBCMAC-IV, and 32 bits for init sequence number */

diff --git a/transform-common.h b/transform-common.h
index 52f606777e53d554cd509651ab0febbffef781de..24ab8dc2bd54f042f79aaeeda920c968768378b3 100644 (file)
--- a/transform-common.h
+++ b/transform-common.h
@@ -61,7 +61,6 @@
        ti->ops.forwards=transform_forward;             \
        ti->ops.reverse=transform_reverse;              \
        ti->ops.destroy=transform_destroy;              \
-       ti->ops.max_start_pad=st->ops.max_start_pad;    \
        ti->keyed=False;
 
 #endif /*TRANSFORM_COMMON_H*/

diff --git a/transform-eax.c b/transform-eax.c
index 31d81714de8a71d69177335a6b6b48151301171d..f881abb9eb424ec444da7ce091e8a038592760f6 100644 (file)
--- a/transform-eax.c
+++ b/transform-eax.c
@@ -294,7 +294,7 @@ static list_t *transform_apply(closure_t *self, struct cloc loc,
        padding_round = 1;
     st->p.padding_mask = padding_round-1;
 
-    st->ops.max_start_pad=0;
+    update_max_start_pad(&transform_max_start_pad, 0);
 
     st->ops.keylen=0;
     st->ops.create=transform_create;

diff --git a/tun.c b/tun.c
index b0bde38ea7405057fe448ac84e4a2d5e593c9bbc..40bf6dd91d8de62c5474f7552d77acd4b2d3e3b7 100644 (file)
--- a/tun.c
+++ b/tun.c
@@ -116,8 +116,8 @@ static void tun_afterpoll(void *sst, struct pollfd *fds, int nfds)
     }
     if (fds[0].revents&POLLIN) {
        BUF_ALLOC(st->buff,"tun_afterpoll");
-       buffer_init(st->buff,st->nl.max_start_pad);
-       l=read(st->fd,st->buff->start,st->buff->len-st->nl.max_start_pad);
+       buffer_init(st->buff,calculate_max_start_pad());
+       l=read(st->fd,st->buff->start,st->buff->len-calculate_max_start_pad());
        if (l<0) {
            fatal_perror("tun_afterpoll: read()");
        }

diff --git a/udp.c b/udp.c
index 12fcfe8c6ec6edb5c0904c1952d36b00c8ddd94f..42fbb1f697dbcfc6491a5c6b56886a9ed670098c 100644 (file)
--- a/udp.c
+++ b/udp.c
@@ -193,12 +193,13 @@ static bool_t udp_sendmsg(void *commst, struct buffer_if *buf,
     uint8_t *sa;
 
     if (st->use_proxy) {
-       sa=buf->start-8;
+       sa=buf_prepend(buf,8);
        memcpy(sa,&dest->sin.sin_addr,4);
        memset(sa+4,0,4);
        memcpy(sa+6,&dest->sin.sin_port,2);
        sendto(st->fd,sa,buf->size+8,0,(struct sockaddr *)&st->proxy,
               sizeof(st->proxy));
+       buf_unprepend(buf,8);
     } else {
        sendto(st->fd, buf->start, buf->size, 0,
               (struct sockaddr *)&dest->sin, sizeof(dest->sin));
@@ -288,7 +289,6 @@ static list_t *udp_apply(closure_t *self, struct cloc loc, dict_t *context,
     st->cl.apply=NULL;
     st->cl.interface=&st->ops;
     st->ops.st=st;
-    st->ops.min_start_pad=0;
     st->ops.request_notify=request_notify;
     st->ops.release_notify=release_notify;
     st->ops.sendmsg=udp_sendmsg;
@@ -323,9 +323,10 @@ static list_t *udp_apply(closure_t *self, struct cloc loc, dict_t *context,
            cfgfatal(st->loc,"udp","proxy must supply ""addr"",port\n");
        }
        st->proxy.sin_port=htons(i->data.number);
-       st->ops.min_start_pad=8;
     }
 
+    update_max_start_pad(&comm_max_start_pad, st->use_proxy ? 8 : 0);
+
     add_hook(PHASE_GETRESOURCES,udp_phase_hook,st);
 
     return new_closure(&st->cl);

diff --git a/util.c b/util.c
index 24dd9a33ebfb375ea3047425e83b8c8bc6b376c2..8c23485c059581cbb2aef2c1d8c40628dbaeecaa 100644 (file)
--- a/util.c
+++ b/util.c
@@ -387,7 +387,7 @@ void send_nak(const struct comm_addr *dest, uint32_t our_index,
              uint32_t their_index, uint32_t msgtype,
              struct buffer_if *buf, const char *logwhy)
 {
-    buffer_init(buf,dest->comm->min_start_pad);
+    buffer_init(buf,calculate_max_start_pad());
     buf_append_uint32(buf,their_index);
     buf_append_uint32(buf,our_index);
     buf_append_uint32(buf,LABEL_NAK);
@@ -419,3 +419,19 @@ void util_module(dict_t *dict)
 {
     add_closure(dict,"sysbuffer",buffer_apply);
 }
+
+void update_max_start_pad(int32_t *our_module_global, int32_t our_instance)
+{
+    if (*our_module_global < our_instance)
+       *our_module_global=our_instance;
+}
+
+int32_t        transform_max_start_pad, comm_max_start_pad;
+
+int32_t calculate_max_start_pad(void)
+{
+    return
+       site_max_start_pad +
+       transform_max_start_pad +
+       comm_max_start_pad;
+}