Re: [PATCH] extensions: masquerade: Add RFC-7597 section 5.1 PSID support

[Florian Westphal <fw@strlen.de>]

Cole Dishington <Cole.Dishington@alliedtelesis.co.nz> wrote:
> Added --psid option to masquerade extension to specify port ranges, as
> described in RFC-7597 section 5.1. The PSID option needs the base field
> in range2, so add version 1 of the masquerade extension.
> 
> Signed-off-by: Cole Dishington <Cole.Dishington@alliedtelesis.co.nz>
> ---
>  extensions/libipt_MASQUERADE.c   | 283 +++++++++++++++++++++++++------
>  include/linux/netfilter/nf_nat.h |   5 +-

Can you add test cases too?
( extensions/libipt_MASQUERADE.t ).

The new option needs to be added to the man page as well that briefly
explains what its doing and what the --psid numbers do (ports? bits?).

> +static void MASQUERADE_help_v1(void)
> +{
> +	printf(
> +"MASQUERADE target options:\n"
> +" --to-ports <port>[-<port>]\n"
> +"				Port (range) to map to.\n"
> +" --random\n"
> +"				Randomize source port.\n"
> +" --random-fully\n"
> +"				Fully randomize source port.\n"

Consider removing the above, you can just call
MASQUERADE_help() before printf(" --psid ...

> +static void range_to_psid_args(struct nf_nat_range2 *r, unsigned int *offset,
> +			       unsigned int *psid, unsigned int *psid_length)
> +{

warning: passing argument 1 of 'range_to_psid_args' discards 'const'
qualifier from pointer target type [-Wdiscarded-qualifiers]
  239 |    range_to_psid_args(r, &offset, &psid, &psid_length);

> +	min = htons(r->min_proto.all);
> +	power_j = htons(r->max_proto.all) - min + 1;
> +	*offset = ntohs(r->base_proto.all);
> +	*psid = (min - *offset) >> _log2(power_j);
> +	*psid_length = _log2(*offset/power_j);
> +}
> +
> +static void parse_psid(const char *arg, struct nf_nat_range2 *r)
> +{
> +	char *end;
> +	unsigned int offset, psid, psid_len;
> +
> +	if (!xtables_strtoui(arg, &end, &offset, 0, UINT16_MAX) || *end != ':' ||
> +	    offset >= (1 << 16))
> +		xtables_param_act(XTF_BAD_VALUE, "MASQUERADE", "PSID settings", arg);
> +
> +	if (!xtables_strtoui(end + 1, &end, &psid, 0, UINT16_MAX) || *end != ':')
> +		xtables_param_act(XTF_BAD_VALUE, "MASQUERADE", "PSID settings", arg);
> +
> +	if (!xtables_strtoui(end + 1, &end, &psid_len, 0, UINT16_MAX) || *end != '\0' ||
> +	    psid_len >= 16)
> +		xtables_param_act(XTF_BAD_VALUE, "MASQUERADE", "PSID settings", arg);

This needs better error checking.  For example, this should
say which of the parameters (offset,len, ...) causes the parse error.

> +	psid = psid << (_log2(offset/(1 << psid_len)));

This results in infinite _log2() loop if offset / 1 << len is 0.