Hi,

On Wed, 16 Sep 2020 12:04:55 +0200
Klemens Nanni <k...@openbsd.org> wrote:
> Using the function verb would reads a bit clearer/more intuitive,
> i.e.

Yes, "if (!rtable_exists($2))" seems better.

>> @@ -5887,17 +5897,37 @@ rdomain_exists(u_int rdomain)
>>  
>>      len = sizeof(info);
>>      if (sysctl(mib, 6, &info, &len, NULL, 0) == -1) {
>> -            if (errno == ENOENT) {
>> +            if (errno == ENOENT)
>>                      /* table nonexistent */
>> -                    return 0;
>> -            }
>> -            err(1, "%s", __func__);
>> -    }
>> -    if (info.rti_domainid == rdomain) {
>> -            found[rdomain] = 1;
>> +                    domainid[rdomain] = RT_TABLEID_MAX;
> This does not look correct, RT_TABLEID_MAX (255) is the biggest *valid*
> id, so you cannot use it to denote a nonexistent routing table.

Good catch.  Thanks,

> Perhaps use `static int domainid[RT_TABLEID_MAX+1]' and `-1' to reflect
> ENOENT?

New diff is using -1 for ENOENT.

Also domainid == 0 is a valid domain id, but previous diff cannot make
a cache of it since 0 is the default value.  So new diff is doing

-       static u_int             found[RT_TABLEID_MAX+1];
+       static struct {
+               int      found;
+               int      domainid;
+       }                        rtables[RT_TABLEID_MAX+1];

to distinguish the default 0 and domainid 0.

ok?


Make pfctl check if the rtable really exists when parsing the config.

Index: sbin/pfctl/parse.y
===================================================================
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.701
diff -u -p -r1.701 parse.y
--- sbin/pfctl/parse.y  28 Jan 2020 15:40:35 -0000      1.701
+++ sbin/pfctl/parse.y  16 Sep 2020 10:40:25 -0000
@@ -392,7 +392,9 @@ int  invalid_redirect(struct node_host *
 u_int16_t parseicmpspec(char *, sa_family_t);
 int     kw_casecmp(const void *, const void *);
 int     map_tos(char *string, int *);
+int     get_domainid(u_int);
 int     rdomain_exists(u_int);
+int     rtable_exists(u_int);
 int     filteropts_to_rule(struct pf_rule *, struct filter_opts *);
 
 TAILQ_HEAD(loadanchorshead, loadanchors)
@@ -1217,6 +1219,10 @@ antispoof_opt    : LABEL label   {
                                yyerror("invalid rtable id");
                                YYERROR;
                        }
+                       else if (!rtable_exists($2)) {
+                               yyerror("rtable %lld does not exist", $2);
+                               YYERROR;
+                       }
                        antispoof_opts.rtableid = $2;
                }
                ;
@@ -2001,6 +2007,10 @@ filter_opt       : USER uids {
                                yyerror("invalid rtable id");
                                YYERROR;
                        }
+                       else if (!rtable_exists($2)) {
+                               yyerror("rtable %lld does not exist", $2);
+                               YYERROR;
+                       }
                        filter_opts.rtableid = $2;
                }
                | DIVERTTO STRING PORT portplain {
@@ -2475,7 +2485,7 @@ if_item           : STRING                        {
                | RDOMAIN NUMBER                {
                        if ($2 < 0 || $2 > RT_TABLEID_MAX)
                                yyerror("rdomain %lld outside range", $2);
-                       else if (rdomain_exists($2) != 1)
+                       else if (!rdomain_exists($2))
                                yyerror("rdomain %lld does not exist", $2);
 
                        $$ = calloc(1, sizeof(struct node_if));
@@ -5868,36 +5878,60 @@ map_tos(char *s, int *val)
 }
 
 int
-rdomain_exists(u_int rdomain)
+get_domainid(u_int rtable)
 {
        size_t                   len;
        struct rt_tableinfo      info;
        int                      mib[6];
-       static u_int             found[RT_TABLEID_MAX+1];
+       static struct {
+               int      found;
+               int      domainid;
+       }                        rtables[RT_TABLEID_MAX+1];
 
-       if (found[rdomain] == 1)
-               return 1;
+       if (rtables[rtable].found)
+               return rtables[rtable].domainid;
 
        mib[0] = CTL_NET;
        mib[1] = PF_ROUTE;
        mib[2] = 0;
        mib[3] = 0;
        mib[4] = NET_RT_TABLE;
-       mib[5] = rdomain;
+       mib[5] = rtable;
 
        len = sizeof(info);
        if (sysctl(mib, 6, &info, &len, NULL, 0) == -1) {
-               if (errno == ENOENT) {
+               if (errno == ENOENT)
                        /* table nonexistent */
-                       return 0;
-               }
-               err(1, "%s", __func__);
-       }
-       if (info.rti_domainid == rdomain) {
-               found[rdomain] = 1;
+                       rtables[rtable].domainid = -1;
+               else
+                       err(1, "%s", __func__);
+       } else
+               rtables[rtable].domainid = info.rti_domainid;
+       rtables[rtable].found = 1;
+
+       return rtables[rtable].domainid;
+}
+
+int
+rdomain_exists(u_int rdomain)
+{
+       int     domainid;
+
+       domainid = get_domainid(rdomain);
+       if (domainid == rdomain)
                return 1;
-       }
        /* rdomain is a table, but not an rdomain */
+       return 0;
+}
+
+int
+rtable_exists(u_int rtable)
+{
+       int     domainid;
+
+       domainid = get_domainid(rtable);
+       if (domainid >= 0)
+               return 1;
        return 0;
 }
 

Reply via email to