jail: Properly restrict the child jail's IPv[46] addresses

[zlei]

Summary

Fixes: eb8dcdeac22d jail: network epoch protection for IP address lists

Test Plan

Run the following script:

#!/bin/sh

ifconfig lo0 inet 172.16.0.1/32 alias
ifconfig lo0 inet 172.16.0.2/32 alias
ifconfig lo0 inet 172.16.0.3/32 alias
ifconfig lo0 inet6 2001:db8::1/128 alias
ifconfig lo0 inet6 2001:db8::2/128 alias
ifconfig lo0 inet6 2001:db8::3/128 alias

jail -c name=parent host.hostname=parent path=/ persist children.max=1 ip4.addr=172.16.0.1 ip4.addr=172.16.0.2 ip6.addr=2001:db8::1 ip6.addr=2001:db8::2
jexec parent /bin/sh -s stdin << EOF
  jail -c name=c1 host.hostname=c1 path=/ persist ip4=inherit ip6=inherit
  sleep 1
  jls -j c1
  jexec c1 ifconfig lo0
EOF

sleep 1

jail -m name=parent ip4.addr=172.16.0.3 ip4.addr=172.16.0.1 ip6.addr=2001:db8::3 ip6.addr=2001:db8::1

sleep 1

echo "After modifying parent jail..."
jls -j parent.c1
jexec parent.c1 ifconfig lo0

Then verify the output:

   JID  IP Address      Hostname                      Path
     8  172.16.0.1      c1                            /
lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> metric 0 mtu 16384
	options=680003<RXCSUM,TXCSUM,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6>
	inet6 2001:db8::1 prefixlen 128
	inet6 2001:db8::2 prefixlen 128
	inet 172.16.0.1 netmask 0xffffffff
	inet 172.16.0.2 netmask 0xffffffff
	groups: lo
	nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>
After modifying parent jail...
   JID  IP Address      Hostname                      Path
     8  172.16.0.3      c1                            /
lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> metric 0 mtu 16384
	options=680003<RXCSUM,TXCSUM,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6>
	inet6 2001:db8::1 prefixlen 128
	inet6 2001:db8::3 prefixlen 128
	inet 172.16.0.1 netmask 0xffffffff
	inet 172.16.0.3 netmask 0xffffffff
	groups: lo
	nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>

Can also verify via DDB show prison

[melifaro]

It would be nice to have the test script similar to the one in the testing section in our auto tests/.
Jails are complex beasts, so increasing the covered code scope would help detect regressions faster

sys/kern/kern_jail.c

817

Would be good to have a comment on why the first address is skipped.
Also: why ips is kept the same?

[zlei]

sys/kern/kern_jail.c

817

Would be good to have a comment on why the first address is skipped.

I think that is the same with my explains in D37871 .

Also: why ips is kept the same?

I'll dig into this.
The logic prior eb8dcdeac22da compares IP address count of prison with its parent, before copying the parent's IP address list

[zlei]

In D37872#860731, @melifaro wrote:

It would be nice to have the test script similar to the one in the testing section in our auto tests/.
Jails are complex beasts, so increasing the covered code scope would help detect regressions faster

I agree!

[zlei]

sys/kern/kern_jail.c

803

While test jail -m to modify parent jail, kernel panics when child jail does not have either IPv4 or IPv6 addresses.

So there're two bugs.

1. IPv[46] addresses inherited from parent is wrong
2. kernel panics when child jail does not have either IPv4 or IPv6 addresses.

@melifaro Shall I fix the second bug separately ?

817

Also: why ips is kept the same?

It works as intended.

Well if there's no user settings, i.e. ip4=inherit or ip6=inherit, then it is safe to copy all addresses from parent to child.

ips is set previously ips = (pr->pr_flags & pr_families[af].ip_flag) ? pip->ips : ppip->ips; .
If there's no user settings, ips == ppip->ips.

If new != NULL, then it is always allocated outside with the size of parent jail's ip adresses.

[zlei]

sys/kern/kern_jail.c

803

While test jail -m to modify parent jail, kernel panics when child jail does not have either IPv4 or IPv6 addresses.

So there're two bugs.

1. IPv[46] addresses inherited from parent is wrong
2. kernel panics when child jail does not have either IPv4 or IPv6 addresses.

@melifaro Shall I fix the second bug separately ?

On second thought, I think it deserves.

[melifaro]

sys/kern/kern_jail.c

803

Whatever is more convenient to you :-)

803

Re panics: was it always the case, or is this something recent?

817

Ack. Thank you for the clarification!

849

Wow. Thank you for catching this!

[zlei]

sys/kern/kern_jail.c

803

Re panics: was it always the case, or is this something recent?

It is introduced by eb8dcdeac22d .

[zlei]

sys/kern/kern_jail.c

803

Re panics: was it always the case, or is this something recent?

It is introduced by eb8dcdeac22d .

See D37906 .

[glebius]

As suggested in D37871 maybe use PR_IP(pip, 0) instead of (pip + 1). That will clearly show that we are copying address arrays not the header structure. However, this is just suggestion, I don't insist on it.

I also second suggestion to combine D37871 and D37872 into one.

[zlei]

In D37872#863169, @glebius wrote:

As suggested in D37871 maybe use PR_IP(pip, 0) instead of (pip + 1). That will clearly show that we are copying address arrays not the header structure. However, this is just suggestion, I don't insist on it.

This is same with D37871.

I also second suggestion to combine D37871 and D37872 into one.

@kp
I've never done that before. Is it a commit with two Differential Revision metadata fields?

[kp]

In D37872#863490, @zlei wrote:

@kp
I've never done that before. Is it a commit with two Differential Revision metadata fields?

That's probably the best way, yes. I doubt that the server-side hooks will do the right thing though, so you may end up having to close one or both reviews manually.

[zlei]

Committed in ddbf879d79d4 .