James Carlson wrote:
> Darren Reed writes:
>> With the extension of the timer for this project to tbe 10th of December,
>> I'm updating the materials submmitted to cover changes required to
>> support the eventual filtering of WiFi packets.
>
> A few quick questions and checks:
Jim,
Thank you very much for your help and comments!
>
> 1. I assume that "net_getlifaddr" in the context of a MAC hook
> actually returns a MAC layer address, and not an IP address as the
> name "lif" might otherwise suggest. Right?
Yes, that's right. For MAC hook we're kind of reusing the same function
though there is actually no "lif" at MAC layer. :-) I'd like to hear your
suggestions on this.
>
> 2. You give the new rule processing as:
>
> [INPUT] -> L2 firewall -> "layer2" IP NAT -> "layer2" IP firewall ->
> ... -> IP NAT -> IP firewall -> { IP } -> IP firewall -> IP NAT -> ...
> -> L2 firewall -> "layer2" IP firewall -> "layer2" IP NAT -> [OUTPUT]
>
> I don't understand why the ordering of operations isn't just
> reversed on output. Assuming the input order is correct (and it
> appears to me to be right), the "L2 firewall" element should be
> the last operation before "[OUTPUT]."
It could be a useful feature to combine filtering on L2 and L3
together, or even trigger conditional IP NAT from a L2 filtering rule,
though they are not supported in the current project scope, down the
road we may want to add it, and these features require that L2 firewall
to be processed before those "layer2" IP rules.
Also, since the packet is received at MAC layer, it might be more
straight forward to do the parsing and matching from the MAC header,
as we need to parse the MAC header first anyway to locate the IP header
start. It also allows a simpler implementation in IPFilter but that
can be considered an implementation issue anyway.
Would the above make sense?
>
> 3. We're defining these bits of syntax ourselves, and we're expecting
> that administrators are going to rely on them for the security of
> their systems. Given that, is "Volatile" the right classification
> for the new "family ether" and "layer2" configuration keywords?
We'll think more about this.
>
> 4. Why is hpe_hdrinfo "void *" rather than "hook_pkt_info_t *"?
For MAC layer, it is actually "mac_header_info_t *".
> Void
> pointers are mildly evil, as they prevent the compiler and lint
> from doing their type-checking jobs. What else would hpe_hdrinfo
> point to?
The major reason for using "void *" here, is that hook_pkt_event_t
is a quite general structure that works with all protocols, thus I
feel it is probably not appropriate to use types that specific to
one protocol, e.g. mac_header_info_t is specific to MAC. Also it
could point to other data structure for protocol other than MAC
layer, though there's not such need right now.
>
> (One very small code review nit: as an argument in a function
> definition [dls_devnet_*name2*name], 'const size_t' doesn't do
> anything that 'size_t' alone wouldn't do.)
>
Thank you, will fix.
Regards,
Zhijun
