| Message ID | 1516865502-20835-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] ptr_ring: fix integer overflow | expand |
On Thu, Jan 25, 2018 at 03:31:42PM +0800, Jason Wang wrote: > We try to allocate one more entry for lockless peeking. The adding > operation may overflow which causes zero to be passed to kmalloc(). > In this case, it returns ZERO_SIZE_PTR without any notice by ptr > ring. Try to do producing or consuming on such ring will lead NULL > dereference. Fix this detect and fail early. > > Fixes: bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can overrun array bounds") > Reported-by: syzbot+87678bcf753b44c39b67@syzkaller.appspotmail.com > Cc: John Fastabend <john.fastabend@gmail.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> Ugh that's just way too ugly. I'll work on dropping the extra + 1 - but calling this function with -1 size is the real source of the bug. Do you know how come we do that? > --- > 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 9ca1726..3f99484 100644 > --- a/include/linux/ptr_ring.h > +++ b/include/linux/ptr_ring.h > @@ -453,6 +453,8 @@ static inline int ptr_ring_consume_batched_bh(struct ptr_ring *r, > > static inline void **__ptr_ring_init_queue_alloc(unsigned int size, gfp_t gfp) > { > + if (unlikely(size + 1 == 0)) > + return NULL; > /* Allocate an extra dummy element at end of ring to avoid consumer head > * or produce head access past the end of the array. Possible when > * producer/consumer operations and __ptr_ring_peek operations run in > -- > 2.7.4
On 2018年01月25日 21:45, Michael S. Tsirkin wrote: > On Thu, Jan 25, 2018 at 03:31:42PM +0800, Jason Wang wrote: >> We try to allocate one more entry for lockless peeking. The adding >> operation may overflow which causes zero to be passed to kmalloc(). >> In this case, it returns ZERO_SIZE_PTR without any notice by ptr >> ring. Try to do producing or consuming on such ring will lead NULL >> dereference. Fix this detect and fail early. >> >> Fixes: bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can overrun array bounds") >> Reported-by:syzbot+87678bcf753b44c39b67@syzkaller.appspotmail.com >> Cc: John Fastabend<john.fastabend@gmail.com> >> Signed-off-by: Jason Wang<jasowang@redhat.com> > Ugh that's just way too ugly. > I'll work on dropping the extra + 1 - but calling this > function with -1 size is the real source of the bug. > Do you know how come we do that? > It looks e.g try to change tx_queue_len to UINT_MAX. And we probably can't prevent user form trying to do this? Thanks
On Thu, Jan 25, 2018 at 10:17:38PM +0800, Jason Wang wrote: > > > On 2018年01月25日 21:45, Michael S. Tsirkin wrote: > > On Thu, Jan 25, 2018 at 03:31:42PM +0800, Jason Wang wrote: > > > We try to allocate one more entry for lockless peeking. The adding > > > operation may overflow which causes zero to be passed to kmalloc(). > > > In this case, it returns ZERO_SIZE_PTR without any notice by ptr > > > ring. Try to do producing or consuming on such ring will lead NULL > > > dereference. Fix this detect and fail early. > > > > > > Fixes: bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can overrun array bounds") > > > Reported-by:syzbot+87678bcf753b44c39b67@syzkaller.appspotmail.com > > > Cc: John Fastabend<john.fastabend@gmail.com> > > > Signed-off-by: Jason Wang<jasowang@redhat.com> > > Ugh that's just way too ugly. > > I'll work on dropping the extra + 1 - but calling this > > function with -1 size is the real source of the bug. > > Do you know how come we do that? > > > > It looks e.g try to change tx_queue_len to UINT_MAX. And we probably can't > prevent user form trying to do this? > > Thanks Right. BTW why net-next? Isn't the crash exploitable in net?
On 2018年01月26日 01:31, Michael S. Tsirkin wrote: > On Thu, Jan 25, 2018 at 10:17:38PM +0800, Jason Wang wrote: >> >> On 2018年01月25日 21:45, Michael S. Tsirkin wrote: >>> On Thu, Jan 25, 2018 at 03:31:42PM +0800, Jason Wang wrote: >>>> We try to allocate one more entry for lockless peeking. The adding >>>> operation may overflow which causes zero to be passed to kmalloc(). >>>> In this case, it returns ZERO_SIZE_PTR without any notice by ptr >>>> ring. Try to do producing or consuming on such ring will lead NULL >>>> dereference. Fix this detect and fail early. >>>> >>>> Fixes: bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can overrun array bounds") >>>> Reported-by:syzbot+87678bcf753b44c39b67@syzkaller.appspotmail.com >>>> Cc: John Fastabend<john.fastabend@gmail.com> >>>> Signed-off-by: Jason Wang<jasowang@redhat.com> >>> Ugh that's just way too ugly. >>> I'll work on dropping the extra + 1 - but calling this >>> function with -1 size is the real source of the bug. >>> Do you know how come we do that? >>> >> It looks e.g try to change tx_queue_len to UINT_MAX. And we probably can't >> prevent user form trying to do this? >> >> Thanks > Right. BTW why net-next? Isn't the crash exploitable in net? > Commit bcecb4bbf88a exists only in net-next. And in net we check r->size before trying to dereference the queue. Thanks
On Fri, Jan 26, 2018 at 11:44:22AM +0800, Jason Wang wrote: > > > On 2018年01月26日 01:31, Michael S. Tsirkin wrote: > > On Thu, Jan 25, 2018 at 10:17:38PM +0800, Jason Wang wrote: > > > > > > On 2018年01月25日 21:45, Michael S. Tsirkin wrote: > > > > On Thu, Jan 25, 2018 at 03:31:42PM +0800, Jason Wang wrote: > > > > > We try to allocate one more entry for lockless peeking. The adding > > > > > operation may overflow which causes zero to be passed to kmalloc(). > > > > > In this case, it returns ZERO_SIZE_PTR without any notice by ptr > > > > > ring. Try to do producing or consuming on such ring will lead NULL > > > > > dereference. Fix this detect and fail early. > > > > > > > > > > Fixes: bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can overrun array bounds") > > > > > Reported-by:syzbot+87678bcf753b44c39b67@syzkaller.appspotmail.com > > > > > Cc: John Fastabend<john.fastabend@gmail.com> > > > > > Signed-off-by: Jason Wang<jasowang@redhat.com> > > > > Ugh that's just way too ugly. > > > > I'll work on dropping the extra + 1 - but calling this > > > > function with -1 size is the real source of the bug. > > > > Do you know how come we do that? > > > > > > > It looks e.g try to change tx_queue_len to UINT_MAX. And we probably can't > > > prevent user form trying to do this? > > > > > > Thanks > > Right. BTW why net-next? Isn't the crash exploitable in net? > > > > Commit bcecb4bbf88a exists only in net-next. Right you are. > And in net we check r->size > before trying to dereference the queue. > > Thanks I was wondering what it's about btw. Does anyone really create 0 size rings?
From: Jason Wang <jasowang@redhat.com> Date: Thu, 25 Jan 2018 15:31:42 +0800 > We try to allocate one more entry for lockless peeking. The adding > operation may overflow which causes zero to be passed to kmalloc(). > In this case, it returns ZERO_SIZE_PTR without any notice by ptr > ring. Try to do producing or consuming on such ring will lead NULL > dereference. Fix this detect and fail early. > > Fixes: bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can overrun array bounds") > Reported-by: syzbot+87678bcf753b44c39b67@syzkaller.appspotmail.com > Cc: John Fastabend <john.fastabend@gmail.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> I'm dropping this because I am to understand that Michael Tsirkin's patch series covers this issue. Let me know if I still need to apply this. Thanks.
On 2018年01月30日 01:01, David Miller wrote: > From: Jason Wang <jasowang@redhat.com> > Date: Thu, 25 Jan 2018 15:31:42 +0800 > >> We try to allocate one more entry for lockless peeking. The adding >> operation may overflow which causes zero to be passed to kmalloc(). >> In this case, it returns ZERO_SIZE_PTR without any notice by ptr >> ring. Try to do producing or consuming on such ring will lead NULL >> dereference. Fix this detect and fail early. >> >> Fixes: bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can overrun array bounds") >> Reported-by: syzbot+87678bcf753b44c39b67@syzkaller.appspotmail.com >> Cc: John Fastabend <john.fastabend@gmail.com> >> Signed-off-by: Jason Wang <jasowang@redhat.com> > I'm dropping this because I am to understand that Michael Tsirkin's patch > series covers this issue. Yes. > > Let me know if I still need to apply this. > > Thanks. No need for this. Thanks
diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h index 9ca1726..3f99484 100644 --- a/include/linux/ptr_ring.h +++ b/include/linux/ptr_ring.h @@ -453,6 +453,8 @@ static inline int ptr_ring_consume_batched_bh(struct ptr_ring *r, static inline void **__ptr_ring_init_queue_alloc(unsigned int size, gfp_t gfp) { + if (unlikely(size + 1 == 0)) + return NULL; /* Allocate an extra dummy element at end of ring to avoid consumer head * or produce head access past the end of the array. Possible when * producer/consumer operations and __ptr_ring_peek operations run in
We try to allocate one more entry for lockless peeking. The adding operation may overflow which causes zero to be passed to kmalloc(). In this case, it returns ZERO_SIZE_PTR without any notice by ptr ring. Try to do producing or consuming on such ring will lead NULL dereference. Fix this detect and fail early. Fixes: bcecb4bbf88a ("net: ptr_ring: otherwise safe empty checks can overrun array bounds") Reported-by: syzbot+87678bcf753b44c39b67@syzkaller.appspotmail.com Cc: John Fastabend <john.fastabend@gmail.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- include/linux/ptr_ring.h | 2 ++ 1 file changed, 2 insertions(+)