[Patch net] ptr_ring: wrap back ->producer in __ptr_ring_swap_queue()

[Patch net] ptr_ring: wrap back ->producer in __ptr_ring_swap_queue()

[Cong Wang]

__ptr_ring_swap_queue() tries to move pointers from the old
ring to the new one, but it forgets to check if ->producer
is beyond the new size at the end of the operation. This leads
to an out-of-bound access in __ptr_ring_produce() as reported
by syzbot.

Reported-by: syzbot+8993c0fa96d57c399735@syzkaller.appspotmail.com
Fixes: 5d49de532002 ("ptr_ring: resize support")
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/linux/ptr_ring.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 6894976b54e3..186cd8e970c7 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -573,6 +573,8 @@ static inline void **__ptr_ring_swap_queue(struct ptr_ring *r, void **queue,
 		else if (destroy)
 			destroy(ptr);
 
+	if (producer >= size)
+		producer = 0;
 	__ptr_ring_set_size(r, size);
 	r->producer = producer;
 	r->consumer_head = 0;
-- 
2.19.2

Re: [Patch net] ptr_ring: wrap back ->producer in __ptr_ring_swap_queue()

[Michael S. Tsirkin]

On Sun, Dec 30, 2018 at 12:43:42PM -0800, Cong Wang wrote:
> __ptr_ring_swap_queue() tries to move pointers from the old
> ring to the new one, but it forgets to check if ->producer
> is beyond the new size at the end of the operation. This leads
> to an out-of-bound access in __ptr_ring_produce() as reported
> by syzbot.
> 
> Reported-by: syzbot+8993c0fa96d57c399735@syzkaller.appspotmail.com
> Fixes: 5d49de532002 ("ptr_ring: resize support")
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  include/linux/ptr_ring.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> index 6894976b54e3..186cd8e970c7 100644
> --- a/include/linux/ptr_ring.h
> +++ b/include/linux/ptr_ring.h
> @@ -573,6 +573,8 @@ static inline void **__ptr_ring_swap_queue(struct ptr_ring *r, void **queue,
>  		else if (destroy)
>  			destroy(ptr);
>  
> +	if (producer >= size)
> +		producer = 0;
>  	__ptr_ring_set_size(r, size);
>  	r->producer = producer;
>  	r->consumer_head = 0;
> -- 
> 2.19.2

To clarify the commit log a little bit:

               if (producer < size)
                        queue[producer++] = ptr;

if the new size is smaller than the old one, we
producer can end up being equal to the new size thus
pointing one beyond the end of the array.

So when we allocated one extra entry it was fine, thus maybe we should rather list:

Fixes: 9fb582b67072 ("Revert "net: ptr_ring: otherwise safe empty checks can overrun array bounds"")


The patch itself is good though. So

Acked-by: Michael S. Tsirkin <mst@redhat.com>

and pls queue this for stable.

-- 
MST

Re: [Patch net] ptr_ring: wrap back ->producer in __ptr_ring_swap_queue()

[Cong Wang]

On Sun, Dec 30, 2018 at 4:52 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> So when we allocated one extra entry it was fine, thus maybe we should rather list:
>
> Fixes: 9fb582b67072 ("Revert "net: ptr_ring: otherwise safe empty checks can overrun array bounds"")
>
>

Good point, I totally forgot we once allocated one extra entry.

However, prior to commit bcecb4bbf88a which introduced the
extra one entry, this bug exists too, IIUC. Commit bcecb4bbf88a
is merged 18 months after commit 5d49de532002 which is
supposed to the one introduced this bug.

On the other hand, it does not harm even if we backport this
to a kernel release with commit bcecb4bbf88a.

Thanks.

Re: [Patch net] ptr_ring: wrap back ->producer in __ptr_ring_swap_queue()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sun, 30 Dec 2018 12:43:42 -0800

> __ptr_ring_swap_queue() tries to move pointers from the old
> ring to the new one, but it forgets to check if ->producer
> is beyond the new size at the end of the operation. This leads
> to an out-of-bound access in __ptr_ring_produce() as reported
> by syzbot.
> 
> Reported-by: syzbot+8993c0fa96d57c399735@syzkaller.appspotmail.com
> Fixes: 5d49de532002 ("ptr_ring: resize support")
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied and queued up for -stable.