On 21/07/09 05:55 PM, Anders Persson wrote:
On Tue, Jul 21, 2009 at 01:58:18PM -0700, Darren Reed wrote:
Anders Persson wrote:
On Tue, Jul 21, 2009 at 07:45:36AM -0700, Darren Reed wrote:
Anders Persson wrote:
On Mon, Jul 06, 2009 at 07:17:14PM -0700, Darren Reed wrote:
I'd like to invite people to review the PF_PACKET port of the
packet capture project. Apologies if some of the Makefile bits
seem like orphans, other changes are embedded with BPF work
that isn't yet ready for review. The primary file requiring review
here is sockmod_pfp.c, but please feel free to review others.
I'd like to receieve any comments by the end of next week
(17th of July 2009.)
http://cr.opensolaris.org/~darrenr/pf_packet_review/
Hi Darren,
The comments below are for sockmod_pfp.c.
- There are a few code paths that use copyin/copyout explicitly, making them
unsuitable for kernel sockets. Is it expected that PF_PACKET will only be
used by userland sockets? There is unfortunately no mechanism for marking
a module as "unavailable" for kernel consumers, which is maybe what is
needed in this case.
Discuss.
It would seem that ddi_copyin/out are normally used in this circumstance,
however the prototype for the sockmod ioctl call does not provide a flags
parameter, like the normal driver ioctl entry point does, meaning
there is no
easy way to communicate to the copy function whether the source is in user
or kernel space. See ddi_copyin(9f). Fixing this problem properly requires
a revision to the socket module API's socksetopt/getsockopt interfaces.
Well, the ioctl sockmod entry point has a 'mode' flag, just like the
driver entry point. So checking for FKIOCTL will tell you whether the
buffer is a kernel address. However, that does not exist for
{set,get}sockopt(). Do you already need to "tweak" applications that
use LSF for them to use PF_PACKET on OpenSolaris? If so, would it be
possible to have the application specify the total size of the filter
when calling SO_ATTACH_FILTER?
The goal of this project is to not require developers to need to do
extra tweaking
to target Solaris - that defeats the purpose of developing a compatible API.
This is starting to sound like an RFE for sockfs to modify
get/setsockopt entry
points for socket modules.
Potentially, yes. The way SO_ATTACH_FILTER is handled is somewhat
unusual (as far as socket options goes), but I guess it mimics
BIOCSETF.
Right.
One more comment on the code:
- line 1155: Should you not make sure that the filter size is not
outrageous before allocating space for it.
Reject.
Just below this, the code calls bpf_validate(). This limits the instruction
set loaded to BPF_MAXINSNS (512). So the worst is that someone with
privilege can ask the kernel to allocate a large amount of memory before
it gets free'd. To implement another check on the size allocated would
either double up on what bpf_validate() does (and increase maintenance
cost of the code) or enforce some other arbitrary limit that is unrelated
to what bpf_validate() does. But to be fair, it's a fair comment given that
the code to bpf_validate() wasn't exposed in this review.
Darren
_______________________________________________
networking-discuss mailing list
[email protected]
