On Fri, Nov 26, 2021 at 11:37:37PM +0100, Alexandr Nedvedicky wrote:
> Hello,
>
> On Fri, Nov 26, 2021 at 01:01:40PM +0100, Claudio Jeker wrote:
> >
> > One more thing to consider, I think the following test in pfi_set_flags():
> >
> > > + if ((p->pfik_flags_new != p->pfik_flags) &&
> > > + (p->pfik_flagrefs == 0))
> > > + pfi_kif_ref(p, PFI_KIF_REF_FLAG);
> >
> > should actually check for the PFI_IFLAG_SKIP flag and not any flag.
> >
> > if (ISSET(p->pfik_flags_new, PFI_IFLAG_SKIP) &&
> > p->pfik_flagrefs == 0)
> > pfi_kif_ref(p, PFI_KIF_REF_FLAG);
> >
> > Same goes for pfi_clear_flags() just in reverse:
> >
> > > + if ((p->pfik_flags_new != p->pfik_flags) &&
> > > + (p->pfik_flagrefs == 1))
> > > + pfi_kif_unref(p, PFI_KIF_REF_FLAG);
> >
> > Should be changed to:
> > if (!ISSET(p->pfik_flags_new, PFI_IFLAG_SKIP) &&
> > p->pfik_flagrefs == 1)
> > pfi_kif_unref(p, PFI_KIF_REF_FLAG);
> >
> > We only want to track the PFI_IFLAG_SKIP flag but not any other flag like
> > PFI_IFLAG_ANY. At least I think we want to do that, but then I guess
> > pfi_set_flags() should only add a kif
> > if (found == 0 && ISSET(flags, PFI_IFLAG_SKIP))
> >
>
> yes it makes sense.
>
> > I don't really like pfi_set_flags() and pfi_clear_flags()
> > and their ioctls DIOCSETIFFLAG and DIOCCLRIFFLAG. There are no checks for
> > valid flag combinations. So anything goes for these functions.
> > Also should the name check not happen in the ioctl handler and return
> > EINVAL for bad input?
> >
>
> the thing is that empty string acts as a kind of wildcard pfi_skip_if()
> matches anything if 'name' is NULL or empty string. So I'm keeping sanity
> check in pfi_set_flags().
>
> But it it still worth to add test for io == NULL to DIOCSETIFFLAG
> and to DIOCCLRIFFLAG to avoid NULL pointer dereference (NULL->pfiio_name)
>
> updated diff is below.
See comments below.
> +void
> +pfi_group_delmember(const char *group, struct ifnet *ifp)
> +{
> + struct pfi_kif *gkif, *ikif;
> +
> + if ((gkif = pfi_kif_get(group, NULL)) == NULL ||
> + (ikif = pfi_kif_get(ifp->if_xname, NULL)) == NULL)
> + panic("%s: pfi_kif_get failed", __func__);
> + ikif->pfik_flags_new = ikif->pfik_flags & ~gkif->pfik_flags;
> +
> + pfi_group_change(group);
> +}
> +
This function is not quite right. For example:
ifconfig vio0 group foo group bar
pfctl -f - <<EOF
set skip on { foo bar }
block return
EOF
ping -qc2 100.64.1.2
PING 100.64.1.2 (100.64.1.2): 56 data bytes
--- 100.64.1.2 ping statistics ---
2 packets transmitted, 2 packets received, 0.0% packet loss
round-trip min/avg/max/std-dev = 0.153/0.178/0.202/0.024 ms
Now lets remove just group bar from the interface.
ifconfig vio0 -group bar
ping -qc2 100.64.1.2
PING 100.64.1.2 (100.64.1.2): 56 data bytes
ping: sendmsg: Permission denied
ping: wrote 100.64.1.2 64 chars, ret=-1
ping: sendmsg: Permission denied
ping: wrote 100.64.1.2 64 chars, ret=-1
pfi_group_delmember() does not take into consideration other groups
(including the interface itself) that may still allow PFI_IFLAG_SKIP.
It just clears the flag.
I think on delete the flag needs to be recalculated after the group has
been removed. Not sure if this is easy possible though.
> --- a/sys/net/pf_ioctl.c
> +++ b/sys/net/pf_ioctl.c
> @@ -2892,6 +2892,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
> struct proc *p)
> case DIOCSETIFFLAG: {
> struct pfioc_iface *io = (struct pfioc_iface *)addr;
>
> + if (io == NULL)
> + return (EINVAL);
I would extend this to also make sure that io->pfiio_flags only sets
PFI_IFLAG_SKIP.
if (io == NULL || io->pfiio_flags & ~PFI_IFLAG_SKIP)
return (EINVAL);
Userland is not allowed to touch PFI_IFLAG_ANY (at least that is my
understanding of the code).
> +
> NET_LOCK();
> PF_LOCK();
> error = pfi_set_flags(io->pfiio_name, io->pfiio_flags);
> @@ -2903,6 +2906,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
> struct proc *p)
> case DIOCCLRIFFLAG: {
> struct pfioc_iface *io = (struct pfioc_iface *)addr;
>
> + if (io == NULL)
> + return (EINVAL);
Same here.
> +
> NET_LOCK();
> PF_LOCK();
> error = pfi_clear_flags(io->pfiio_name, io->pfiio_flags);
--
:wq Claudio
