Re: git: fb8ef16bab0d - main - IPv4: correct limit on loopback_prefix

On 21 Jul 2022, at 12:21, Rodney W. Grimes wrote:

>> On 21 Jul 2022, at 11:21, Rodney W. Grimes wrote:
>>
>>>> The branch main has been updated by karels:
>>>>
>>>> URL: https://cgit.FreeBSD.org/src/commit/?id=fb8ef16bab0d23e185deed5a6b2e44e72ad53d43
>>>>
>>>> commit fb8ef16bab0d23e185deed5a6b2e44e72ad53d43
>>>> Author:     Mike Karels <karels@FreeBSD.org>
>>>> AuthorDate: 2022-07-21 13:10:15 +0000
>>>> Commit:     Mike Karels <karels@FreeBSD.org>
>>>> CommitDate: 2022-07-21 14:38:17 +0000
>>>>
>>>>     IPv4: correct limit on loopback_prefix
>>>>
>>>>     Commit efe58855f3ea allowed the net.inet.ip.loopback_prefix value
>>>>     to be 32.  However, with a 32-bit mask, 127.0.0.1 is not included
>>>>     in the reserved loopback range, which should not be allowed.
>>>>     Change the max prefix length to 31.
>>>
>>> Hummm... 127.0.0.1/32 specifices exactly and ONLY 127.0.0.1, and
>>> this should be fine.  Looking at the mask calculated below with
>>> loopback_prefix=32 this should yeild a mask of 0xffffffff, which
>>> appears to be exactly what is correct.  What DOES become an issue
>>> when /32 is used is that the loopback ROUTE 127.0.0.0/32 is wrong
>>> now, but then with a /32 you dont need a network route, as you
>>> should have a host route to exactly 127.0.0.1.
>>>
>>> Can you be more descriptive on what problem arrose with /32?
>>
>> You are thinking about this the way I did originally; but the mask
>> doesn?t apply to 127.0.0.1 directly.  The test is
>>
>> #define IN_LOOPBACK(i) \
>>     (((in_addr_t)(i) & V_in_loopback_mask) == 0x7f000000)
>>
>> So if considering whether to forward 127.0.0.1, we?ll incorrectly
>> say it?s OK if the prefixlen is 32 (mask of 255.255.255.255).
>> In that case, only 127.0.0.0 is considered loopback.
>>
>> John Gilmore pointed out the problem.
>
> I see this issue now, but something is bugging me about
> being forced to use a /31 when a /32 *would* work if the
> 0x7f000000 was actually 0x7f000001, but that fails for
> other cases.
>
> Note oddly the code would work with /32 if I decided to use
> 127.0.0.0/32 as the IP address on lo0 so should the expression
> become:
>
> #define IN_LOOPBACK(i) \
>     (((in_addr_t)(i) & V_in_loopback_mask) == (0x7f000001 & V_in_loopback_mask))

John mentioned that, also a little trickier version using ^1,
but the question is, should 127.0.0.0 should be considered
loopback, or could it be forwarded etc?  I think it is better
if neither 127.0.0.0 nor 127.0.0.1 was allowed to be forwarded.
However, a prefix length of more than 16 doesn’t conform with
current standards or the current draft, so it’s a somewhat
academic issue.

		Mike

>>>> ---
>>>>  sys/netinet/in.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/sys/netinet/in.c b/sys/netinet/in.c
>>>> index c3880c4ba983..1c44623bdec1 100644
>>>> --- a/sys/netinet/in.c
>>>> +++ b/sys/netinet/in.c
>>>> @@ -297,7 +297,7 @@ sysctl_loopback_prefixlen(SYSCTL_HANDLER_ARGS)
>>>>  	error = sysctl_handle_int(oidp, &preflen, 0, req);
>>>>  	if (error || !req->newptr)
>>>>  		return (error);
>>>> -	if (preflen < 8 || preflen > 32)
>>>> +	if (preflen < 8 || preflen > 31)
>>>>  		return (EINVAL);
>>>>  	V_in_loopback_mask = 0xffffffff << (32 - preflen);
>>>>  	return (0);
>>>>
>>>
>>> -- 
>>> Rod Grimes                                                 rgrimes@freebsd.org
>>
>>
>
> -- 
> Rod Grimes                                                 rgrimes@freebsd.org