svn commit: r296025 - head/sys/netpfil/pf

[Kristof Provost]

On 2016-02-25 12:07:57 (+0200), Konstantin Belousov <kostikbel at gmail.com> wrote:
> On Thu, Feb 25, 2016 at 10:17:41AM +0100, Kristof Provost wrote:
> > On 2016-02-24 23:47:55 (-0800), Conrad Meyer <cem at FreeBSD.org> wrote:
> > > On Wed, Feb 24, 2016 at 11:41 PM, Adrian Chadd <adrian.chadd at gmail.com> wrote:
> > > > .. what's capping totlen so one doesn't run out of memory?
> > > 
> > > There was a DoS vector before (user controlled io->pfrio_size) and
> > > basically the same DoS vector now (either of io->pfrio_size or
> > > io->pfrio_size2).  This change isn't a regression.  Still, it should
> > > be fixed.
> > > 
> > It's an M_WAITOK allocation, so if the user asks for more memory than is
> > available the thread will sleep. I'd assumed that if the user terminates
> > the thread the sleep will wake, the allocation will fail and the ioctl()
> > will return an error.
> M_WAITOK allocations still panic when requested amount of KVA is
> unreasonable.
> 
> I am curious what do you mean by 'user terminating the thread'. The
> sleep in malloc() is uninterruptible by signal, and user does not have
> any other way to disturb the execution.
> 
Ah, so my assumptions about malloc() are incorrect. That's good to know.

> > Perhaps we should do what OpenBSD do, and not allocate the temporary
> > buffer at all. They copy in/out the individual entries one by one. On
> > the other hand, one could still exhaust memory by inserting large
> > numbers of addresses in the table.
> But note that accesses to the user memory may fault, which puts whole
> VM and VFS subsystems (and possibly the network as well, if user
> supplied address is backed by mapped NFS file) after the locks owned
> at the moment of copyin() call.
> 
That's a good point. We could handle those accesses failing, but
sleeping with the PF_RULES_WLOCK held would be a bad idea.

I suppose we could just change M_WAITOK into M_NOWAIT and return ENOMEM
if it fails. The only downside I can see is that it's more likely for a
call to fail if there's not a lot of free memory.

There's actually a decent number of cases where that'd have to be done,
but it doesn't look particularly hard to do.

Regards,
Kristof