Re: [PATCH net] ip_gre: validate csum_start only if CHECKSUM_PARTIAL

On Sat, Sep 4, 2021 at 5:54 PM Alexander Duyck
[off-list ref] wrote:

On Sat, Sep 4, 2021 at 2:40 PM Willem de Bruijn
[off-list ref] wrote:

quoted

On Sat, Sep 4, 2021 at 11:37 AM Alexander Duyck
[off-list ref] wrote:

quoted

On Sat, Sep 4, 2021 at 7:46 AM Willem de Bruijn
[off-list ref] wrote:

quoted

On Fri, Sep 3, 2021 at 7:27 PM Alexander Duyck
[off-list ref] wrote:

quoted

On Fri, Sep 3, 2021 at 12:38 PM Willem de Bruijn
[off-list ref] wrote:

quoted

<snip>

quoted

quoted

whereas if the offset is stored somewhere in the unstripped data we
could then drop the packet and count it as a drop without having to
modify the frame via the skb_pull.

This is a broader issue that userspace can pass any csum_start as long
as it is within packet bounds. We could address it here specifically
for the GRE header. But that still leaves many potentially bad offsets
further in the packet in this case, and all the other cases. Checking
that specific header seems a bit arbitrary to me, and might actually
give false confidence.

We could certainly move the validation from gre_handle_offloads to
before skb_pull, to make it more obvious *why* the check exists.

Agreed. My main concern is that the csum_start is able to be located
somewhere where the userspace didn't write. For the most part the
csum_start and csum_offset just needs to be restricted to the regions
that the userspace actually wrote to.

I don't quite follow. Even with this bug, the offset is somewhere userspace
wrote. That data is just pulled.

Sorry, I was thinking of the SOCK_DGRAM case where the header is added
via a call to dev_hard_header().

quoted

quoted

quoted

quoted

Maybe for those
cases we need to look at adding an unsigned int argument to
virtio_net_hdr_to_skb in which we could pass 0 for the unused case or
dev->hard_header_len in the cases where we have something like
af_packet that is transmitting over an ipgre tunnel. The general idea
is to prevent these virtio_net_hdr_to_skb calls from pointing the
csum_start into headers that userspace was not responsible for
populating.

One issue with that is that dev->hard_header_len itself is imprecise
for protocols with variable length link layer headers. There, too, we
have had a variety of bug fixes in the past.

It also adds cost to every user of virtio_net_hdr, while we only know
one issue in a rare case of the IP_GRE device.

Quick question, the assumption is that the checksum should always be
performed starting no earlier than the transport header right? Looking
over virtio_net_hdr_to_skb it looks like it is already verifying the
transport header is in the linear portion of the skb. I'm wondering if
we couldn't just look at adding a check to verify the transport offset
is <= csum start? We might also be able to get rid of one of the two
calls to pskb_may_pull by doing that.

Are you referring to this part in the .._NEEDS_CSUM branch?

                if (!skb_partial_csum_set(skb, start, off))
                        return -EINVAL;

                p_off = skb_transport_offset(skb) + thlen;
                if (!pskb_may_pull(skb, p_off))
                        return -EINVAL;

skb_partial_csum_set is actually what sets the transport offset,
derived from start.

Ugh, I had overlooked that as I was more focused on the
skb_probe_transport_header calls in the af_packet code.

So we can have both the transport offset and the csum_start in a
region that gets stripped by the ipgre code. Worse yet the inner
transport header will also be pointing somewhere outside of the
encapsulated region when we pass it off to skb_reset_inner_headers().

Maybe it would make sense to just have the check look into the
transport offset instead of csum start as that way you are essentially
addressing two possible issues instead of one, and it would
effectively combine multiple checks as the uninitialized value is ~0
which should always be greater than "skb_headroom + tunnel->hlen +
sizeof(struct iphdr)". I think you mentioned before placing a check
just before you make the call to skb_pull in the GRE transmit path.
Doing that we would at least reduce the impact as it would only apply
in the header_ops case in ipgre_xmit instead of being applied to all
the transmit paths which don't perform the pull.

Do you mean

        if (dev->header_ops) {
+               int pull_len = tunnel->hlen + sizeof(struct iphdr);
+
                if (skb_cow_head(skb, 0))
                        goto free_skb;

                tnl_params = (const struct iphdr *)skb->data;

+               if (pull_len > skb_transport_offset(skb))
+                       goto free_skb;
+
                /* Pull skb since ip_tunnel_xmit() needs skb->data pointing
                 * to gre header.
                 */
-               skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
+               skb_pull(skb, pull_len);
                skb_reset_mac_header(skb);

plus then

 static int gre_handle_offloads(struct sk_buff *skb, bool csum)
 {
-       /* Local checksum offload requires csum offload of the inner packet */
-       if (csum && skb->ip_summed == CHECKSUM_PARTIAL &&
-           skb_checksum_start(skb) < skb->data)
-               return -EINVAL;
-
        return iptunnel_handle_offloads(skb, csum ? SKB_GSO_GRE_CSUM :
SKB_GSO_GRE);
 }

Yes, this is what I was thinking. We will also need an IPv6 version of
this as well, and may want to add a comment clarifying that this is to
prevent us from pointing inner offsets at pulled headers.

It lets us drop the csum, ipsummed, and csum_start checks in favor of
just the skb_transport_offset comparison which should be a net win
since it reduces the number of paths the code is encountered in, and
reduces the number of checks to just 1.

Okay. Yes, this looks better to me too. Thanks.

Do you want to submit it? Or I can do it, either way.