[PATCH bpf] bpf: fix several offset tests in bpf_msg_pull_data

[PATCH bpf] bpf: fix several offset tests in bpf_msg_pull_data

[Daniel Borkmann]

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>
---
 net/core/filter.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 7a24309..ec4d67c 100644
--- 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));
-- 
2.9.5

Re: [PATCH bpf] bpf: fix several offset tests in bpf_msg_pull_data

[Alexei Starovoitov]

On Tue, Aug 28, 2018 at 04:15:35PM +0200, Daniel Borkmann wrote:
> 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>

Applied to bpf tree, Thanks