netdev - Re: [Patch net] ptr_ring: wrap back ->producer in __ptr_ring_swap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 30 Dec 2018 19:52:35 -0500
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     netdev@...r.kernel.org, John Fastabend <john.fastabend@...il.com>,
        Jason Wang <jasowang@...hat.com>
Subject: Re: [Patch net] ptr_ring: wrap back ->producer in
 __ptr_ring_swap_queue()

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@...kaller.appspotmail.com
> Fixes: 5d49de532002 ("ptr_ring: resize support")
> Cc: "Michael S. Tsirkin" <mst@...hat.com>
> Cc: John Fastabend <john.fastabend@...il.com>
> Cc: Jason Wang <jasowang@...hat.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.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@...hat.com>

and pls queue this for stable.

-- 
MST