On Mon, Feb 06, 2023 at 09:37:47PM +0100, Alexandr Nedvedicky wrote:
> if we want to allow firewall administrator to specify a match
> on icmptype 255 then extending type from uint8_t to uint16_t
> is the right change.
>
> another option is to change logic here to allow matching icmptype in
> range <0, 254>
Although type 255 is reserved and code 255 not assigned, it seems
useful if pf supports them. This would allow to block such crap.
Or someone might filter it in an experimental environment.
> either way is fine for me. however my preference is leaning
> towards a type change.
Not allowing 255 would be confusing. The special meaning of 0 and
the limited range to 254 is an implementation datail that should
not be exposed to the user.
> note diff also rops pad[2] member to compensate for change
> of uint8_t to uin16_t for type, code members. I'm not sure
> about this bit hence I'd like to bring it up here.
This creates an ABI change. People have to recompile their pfctl.
I think we never guarantee this level of compatibility.
OK bluhm@
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
> index 2c5a49772ff..898bded8c24 100644
> --- a/sbin/pfctl/parse.y
> +++ b/sbin/pfctl/parse.y
> @@ -134,8 +134,8 @@ struct node_gid {
> };
>
> struct node_icmp {
> - u_int8_t code;
> - u_int8_t type;
> + u_int16_t code; /* aux. value 256 is legit */
> + u_int16_t type; /* aux. value 256 is legit */
> u_int8_t proto;
> struct node_icmp *next;
> struct node_icmp *tail;
> diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
> index 1453bc35c04..1efb1b5221c 100644
> --- a/sys/net/pfvar.h
> +++ b/sys/net/pfvar.h
> @@ -572,8 +572,8 @@ struct pf_rule {
> u_int8_t keep_state;
> sa_family_t af;
> u_int8_t proto;
> - u_int8_t type;
> - u_int8_t code;
> + u_int16_t type; /* aux. value 256 is legit */
> + u_int16_t code; /* aux. value 256 is legit */
> u_int8_t flags;
> u_int8_t flagset;
> u_int8_t min_ttl;
> @@ -592,7 +592,6 @@ struct pf_rule {
> u_int8_t set_prio[2];
> sa_family_t naf;
> u_int8_t rcvifnot;
> - u_int8_t pad[2];
>
> struct {
> struct pf_addr addr;
