bpf: fix several offset tests in bpf_msg_pull_data

While recently going over bpf_msg_pull_data(), I noticed three
issues which are fixed in here:

1) When we attempt to find the first scatterlist element (sge)
   for the start offset, we add len to the offset before we check
   for start < offset + len, whereas it should come after when
   we iterate to the next sge to accumulate the offsets. For
   example, given a start offset of 12 with a sge length of 8
   for the first sge in the list would lead us to determine this
   sge as the first sge thinking it covers first 16 bytes where
   start is located, whereas start sits in subsequent sges so
   we would end up pulling in the wrong data.

2) After figuring out the starting sge, we have a short-cut test
   in !msg->sg_copy[i] && bytes <= len. This checks whether it's
   not needed to make the page at the sge private where we can
   just exit by updating msg->data and msg->data_end. However,
   the length test is not fully correct. bytes <= len checks
   whether the requested bytes (end - start offsets) fit into the
   sge's length. The part that is missing is that start must not
   be sge length aligned. Meaning, the start offset into the sge
   needs to be accounted as well on top of the requested bytes
   as otherwise we can access the sge out of bounds. For example
   the sge could have length of 8, our requested bytes could have
   length of 8, but at a start offset of 4, so we also would need
   to pull in 4 bytes of the next sge, when we jump to the out
   label we do set msg->data to sg_virt(&sg[i]) + start - offset
   and msg->data_end to msg->data + bytes which would be oob.

3) The subsequent bytes < copy test for finding the last sge has
   the same issue as in point 2) but also it tests for less than
   rather than less or equal to. Meaning if the sge length is of
   8 and requested bytes of 8 while having the start aligned with
   the sge, we would unnecessarily go and pull in the next sge as
   well to make it private.

Fixes: 015632bb30da ("bpf: sk_msg program helper bpf_sk_msg_pull_data")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>

diff --git a/net/core/filter.c b/net/core/filter.c
index 7a2430945c71998615ac008750cdc9ebf4a5c1d2..ec4d67c0cf0cd323299bdc33da6c52430455193c 100644 (file)
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2286,10 +2286,10 @@ BPF_CALL_4(bpf_msg_pull_data,
           struct sk_msg_buff *, msg, u32, start, u32, end, u64, flags)
 {
        unsigned int len = 0, offset = 0, copy = 0;
+       int bytes = end - start, bytes_sg_total;
        struct scatterlist *sg = msg->sg_data;
        int first_sg, last_sg, i, shift;
        unsigned char *p, *to, *from;
-       int bytes = end - start;
        struct page *page;
 
        if (unlikely(flags || end <= start))
@@ -2299,9 +2299,9 @@ BPF_CALL_4(bpf_msg_pull_data,
        i = msg->sg_start;
        do {
                len = sg[i].length;
-               offset += len;
                if (start < offset + len)
                        break;
+               offset += len;
                i++;
                if (i == MAX_SKB_FRAGS)
                        i = 0;
@@ -2310,7 +2310,11 @@ BPF_CALL_4(bpf_msg_pull_data,
        if (unlikely(start >= offset + len))
                return -EINVAL;
 
-       if (!msg->sg_copy[i] && bytes <= len)
+       /* The start may point into the sg element so we need to also
+        * account for the headroom.
+        */
+       bytes_sg_total = start - offset + bytes;
+       if (!msg->sg_copy[i] && bytes_sg_total <= len)
                goto out;
 
        first_sg = i;
@@ -2330,12 +2334,12 @@ BPF_CALL_4(bpf_msg_pull_data,
                i++;
                if (i == MAX_SKB_FRAGS)
                        i = 0;
-               if (bytes < copy)
+               if (bytes_sg_total <= copy)
                        break;
        } while (i != msg->sg_end);
        last_sg = i;
 
-       if (unlikely(copy < end - start))
+       if (unlikely(bytes_sg_total > copy))
                return -EINVAL;
 
        page = alloc_pages(__GFP_NOWARN | GFP_ATOMIC, get_order(copy));