On 2018年01月18日 13:09, Willem de Bruijn wrote:
Also, a packet can just as easily spoof an esp packet.
See also the discussion in the Link above.
Then we probably need check there.
Adding a check in every gso handler that does not expect packets
with SKB_GSO_DODGY source seems backwards to me.
A packet with gso_type SKB_GSO_TCPV4 should never be
delivered to an sctp handler, say. We must check before passing
to a handler whether the packet matches the callback.
That's the case for trusted source. For dodgy source, we can't assume that.
We can address this specific issue in segmentation by placing a check
analogous to skb_validate_dodgy_gso in the network layer. Something
like this (but with ECN option support):
@@ -1258,6 +1258,22 @@ struct sk_buff *inet_gso_segment(struct sk_buff
*skb,
skb_reset_transport_header(skb);
+ gso_type = skb_shinfo(skb)->gso_type;
+ if (gso_type & SKB_GSO_DODGY) {
+ switch (gso_type & (SKB_GSO_TCPV4 | SKB_GSO_UDP)) {
+ case SKB_GSO_TCPV4:
+ if (proto != IPPROTO_TCP)
+ goto out;
+ break;
+ case SKB_GSO_UDP:
+ if (proto != IPPROTO_UDP)
+ goto out;
+ break;
+ default:
+ goto out;
+ }
+ }
This implements subset of function for codes which was removed by the commit
I mentioned below.
No, as I explain above, it performs a different check.
[...]
For performance reason. I think we should delay the check or segmentation
as
much as possible until it was really needed.
Going through segmentation is probably as expensive as flow dissector,
if not more so because of the indirect branches.
I think we don't even need to care about this consider the evil packet
should be rare.
How does frequency matter when a single packet can crash a host?
I mean consider we had fix the crash, we don't care how expensive do we
spot this.
And what you propose here is just a very small subset of the
necessary checking, more comes at gso header checking. So even if we care
performance, it only help for some specific case.
It also fixed the bug that Eric sent a separate patch for, as that did
not dissect as a valid TCP packet, either.
I may miss something but how did this patch protects an evil thoff?
And now we have two
pieces of code that need to be hardened against malicious packets.
To me fixing the exist path is a good balance between security and
performance.
But I did overlook the guest to guest communication, with virtual devices
that can set NETIF_F_GSO_ROBUST and so do not enter the stack. That
means that one guest can send misrepresented packets to another. Is that
preferable to absorbing the cost of validation?
The problem is that guests and hosts don't trust each other. Even if one
packet has already been validated by one side, it must be validated again by
another side when needed. Doing validation twice is meaningless in this
case.
Ack, agreed that guests need to have defensive coding of themselves.
Then it's fine to pass packets to guests where the gso_type does not
match the contents.
The current patch does not actually deprecate the path through the
segmentation layer. So it will indeed add overhead. We would have to
remove the DODGY logic in net-next.
I don't get the point of removing this.
One additional possible optimization
is to switch to flow_keys_buf_dissector, which does not dissect as deeply.
I did that in an earlier patch; it should be sufficient for this purpose.
I suspect the cost is still noticeable, and consider we may support kind of
tunnel offload in the future.
We should first assume the correctness of metadata provided by untrusted
source and validate it before we really want to use it. Validate them during
entry means we assume they are wrong, then there's no need for most of the
fields of virtio-net header,
If you always need to use the data, then you always validate. In which case
you want to validate early, as there will be less vulnerable code before
validation.
But I see what I think is your point: in guest to guest communication we
do not need to validate at all, so early validation adds unnecessary cost
for this important use case. That's a fair argument for validating later and
only when needed (i.e., a path without NETIF_F_GSO_ROBUST).
Yes, and looking at Herbert's commit log for 576a30eb6453 ("[NET]: Added
GSO header verification"). It was intended do the verification just
before gso.
Thanks
