On Wed, Apr 26, 2023 at 01:47:31PM +0200, Alexandr Nedvedicky wrote:
> Hello,
>
> </snip>
> On Wed, Apr 26, 2023 at 11:37:58AM +1000, David Gwynne wrote:
> > > fail:
> > > - if (flags & FWRITE)
> > > - rw_exit_write(&pfioctl_rw);
> > > - else
> > > - rw_exit_read(&pfioctl_rw);
> > > + rw_exit_write(&pfioctl_rw);
> >
> > i dont think having the open mode flags affect whether you take a shared
> > or exclusive lock makes sense anyway, so im happy with these bits.
>
> while updating my diff I've noticed pfclose() needs
> same change: dropping 'flags & FWRITE' test. This is
> fixed i new diff below.
cool.
>
> </snip>
> >
> > int unit = minor(dev);
> >
> > if (unit & ((1 << CLONE_SHIFT) - 1))
> > return (ENXIO);
> >
> > this has some ties into the second issue, which is that we shouldn't
> > use the pid as an identifier for state across syscalls like this
> > in the kernel. the reason is that userland could be weird or buggy
> > (or hostile) and fork in between creating a transaction and ending
> > a transaction, but it will still have the fd it from from open(/dev/pf).
> > instead, we should be using the whole dev_t or the minor number,
> > as long as it includes the bits that the cloning infrastructure
> > uses, as the identifier.
>
> in new diff I'm using a minor(dev) to identify transaction owner.
> >
> > i would also check that the process^Wminor number that created a
> > transaction is the same when looking up a transaction in pf_find_trans.
>
> the pf_find_trans() is a dead code in diff below as it is
> not being used there. Just to make sure I got things right
> so I can update '3/3' diff in stack accordingly. Let's assume
> snippet below comes from pfioctl() function
>
> int
> pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p) {
> ...
> t = pf_find_trans(ticket);
> if (t == NULL)
> return (ENXIO);
>
> KASSERT(t->pft_unit == minor(dev));
userland controls the ticket value that's passed to the ioctl, so if
it deliberately passes a ticket another fd/minor owns it can trigger
this assert, so i don't think this is a good idea. imagine a program
that opens /dev/pf twice, opens a transaction on one fd, and then
uses the ticket in the other.
> is that kind of check in KASSET() something you have on your mind?
> perhaps I can trade KASSERT() to regular code:
>
> if (t->pft_unit != minor(dev))
> return (EPERM);
i would pass the dev/minor/unit to pf_find_trans() and compare along
with the ticket value as a matter of course. returning a different
errno if the minor is different is unecessary.
>
> thank you for pointers to bpf.c updated diff is below.
>
> regards
> sashan
>
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> index 1141069dcf6..b9904c545c5 100644
> --- a/sys/net/pf_ioctl.c
> +++ b/sys/net/pf_ioctl.c
> @@ -117,6 +117,11 @@ void pf_qid_unref(u_int16_t);
> int pf_states_clr(struct pfioc_state_kill *);
> int pf_states_get(struct pfioc_states *);
>
> +struct pf_trans *pf_open_trans(uint32_t);
> +struct pf_trans *pf_find_trans(uint64_t);
> +void pf_free_trans(struct pf_trans *);
> +void pf_rollback_trans(struct pf_trans *);
> +
> struct pf_rule pf_default_rule, pf_default_rule_new;
>
> struct {
> @@ -168,6 +173,8 @@ int pf_rtlabel_add(struct
> pf_addr_wrap *);
> void pf_rtlabel_remove(struct pf_addr_wrap *);
> void pf_rtlabel_copyout(struct pf_addr_wrap *);
>
> +uint64_t trans_ticket = 1;
> +LIST_HEAD(, pf_trans) pf_ioctl_trans =
> LIST_HEAD_INITIALIZER(pf_trans);
>
> void
> pfattach(int num)
> @@ -293,6 +300,28 @@ pfopen(dev_t dev, int flags, int fmt, struct proc *p)
> int
> pfclose(dev_t dev, int flags, int fmt, struct proc *p)
> {
> + struct pf_trans *w, *s;
> + LIST_HEAD(, pf_trans) tmp_list;
> + uint32_t unit = minor(dev);
> +
> + if (minor(dev) >= 1)
> + return (ENXIO);
gerhard already spotted this one.
> +
> + LIST_INIT(&tmp_list);
> + rw_enter_write(&pfioctl_rw);
> + LIST_FOREACH_SAFE(w, &pf_ioctl_trans, pft_entry, s) {
> + if (w->pft_unit == unit) {
> + LIST_REMOVE(w, pft_entry);
> + LIST_INSERT_HEAD(&tmp_list, w, pft_entry);
> + }
> + }
> + rw_exit_write(&pfioctl_rw);
> +
> + while ((w = LIST_FIRST(&tmp_list)) != NULL) {
> + LIST_REMOVE(w, pft_entry);
> + pf_free_trans(w);
> + }
> +
> return (0);
> }
>
> @@ -1142,10 +1171,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int
> flags, struct proc *p)
> return (EACCES);
> }
>
> - if (flags & FWRITE)
> - rw_enter_write(&pfioctl_rw);
> - else
> - rw_enter_read(&pfioctl_rw);
> + rw_enter_write(&pfioctl_rw);
>
> switch (cmd) {
>
> @@ -3022,10 +3048,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int
> flags, struct proc *p)
> break;
> }
> fail:
> - if (flags & FWRITE)
> - rw_exit_write(&pfioctl_rw);
> - else
> - rw_exit_read(&pfioctl_rw);
> + rw_exit_write(&pfioctl_rw);
>
> return (error);
> }
> @@ -3244,3 +3267,52 @@ pf_sysctl(void *oldp, size_t *oldlenp, void *newp,
> size_t newlen)
>
> return sysctl_rdstruct(oldp, oldlenp, newp, &pfs, sizeof(pfs));
> }
> +
> +struct pf_trans *
> +pf_open_trans(uint32_t unit)
> +{
> + static uint64_t ticket = 1;
> + struct pf_trans *t;
> +
> + rw_assert_wrlock(&pfioctl_rw);
> +
> + t = malloc(sizeof(*t), M_TEMP, M_WAITOK);
> + memset(t, 0, sizeof(struct pf_trans));
> + t->pft_unit = unit;
> + t->pft_ticket = ticket++;
> +
> + LIST_INSERT_HEAD(&pf_ioctl_trans, t, pft_entry);
> +
> + return (t);
> +}
> +
> +struct pf_trans *
> +pf_find_trans(uint64_t ticket)
> +{
> + struct pf_trans *t;
> +
> + rw_assert_anylock(&pfioctl_rw);
> +
> + LIST_FOREACH(t, &pf_ioctl_trans, pft_entry) {
> + if (t->pft_ticket == ticket)
> + break;
> + }
> +
> + return (t);
> +}
> +
> +void
> +pf_free_trans(struct pf_trans *t)
> +{
> + free(t, M_TEMP, sizeof(*t));
> +}
> +
> +void
> +pf_rollback_trans(struct pf_trans *t)
> +{
> + if (t != NULL) {
> + rw_assert_wrlock(&pfioctl_rw);
> + LIST_REMOVE(t, pft_entry);
> + pf_free_trans(t);
> + }
> +}
> diff --git a/sys/net/pfvar_priv.h b/sys/net/pfvar_priv.h
> index 38fff6473aa..5af2027733a 100644
> --- a/sys/net/pfvar_priv.h
> +++ b/sys/net/pfvar_priv.h
> @@ -322,6 +322,17 @@ enum {
>
> extern struct cpumem *pf_anchor_stack;
>
> +enum pf_trans_type {
> + PF_TRANS_NONE,
> + PF_TRANS_MAX
> +};
> +
> +struct pf_trans {
> + LIST_ENTRY(pf_trans) pft_entry;
> + uint32_t pft_unit; /* process id */
if you're tweaking the diff anyway, can you tweak this comment? ^
> + uint64_t pft_ticket;
> + enum pf_trans_type pft_type;
> +};
> extern struct task pf_purge_task;
> extern struct timeout pf_purge_to;
>
