[PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[Thomas Huth]

The XHCI code could enter an endless loop in case the guest points
QEMU to fetch TRBs from invalid memory areas. Fix it by properly
checking the return value of dma_memory_read().

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/646
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/usb/hcd-xhci.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 296cc6c8e6..63d428a444 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -21,6 +21,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/timer.h"
+#include "qemu/log.h"
 #include "qemu/module.h"
 #include "qemu/queue.h"
 #include "migration/vmstate.h"
@@ -679,8 +680,12 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
 
     while (1) {
         TRBType type;
-        dma_memory_read(xhci->as, ring->dequeue, trb, TRB_SIZE,
-                        MEMTXATTRS_UNSPECIFIED);
+        if (dma_memory_read(xhci->as, ring->dequeue, trb, TRB_SIZE,
+                            MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory access failed!\n",
+                          __func__);
+            return 0;
+        }
         trb->addr = ring->dequeue;
         trb->ccs = ring->ccs;
         le64_to_cpus(&trb->parameter);
@@ -727,8 +732,12 @@ static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
 
     while (1) {
         TRBType type;
-        dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE,
-                        MEMTXATTRS_UNSPECIFIED);
+        if (dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE,
+                        MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory access failed!\n",
+                          __func__);
+            return -length;
+        }
         le64_to_cpus(&trb.parameter);
         le32_to_cpus(&trb.status);
         le32_to_cpus(&trb.control);
-- 
2.31.1

Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[Peter Maydell]

On Tue, 2 Aug 2022 at 14:53, Thomas Huth <thuth@redhat.com> wrote:
>
> The XHCI code could enter an endless loop in case the guest points
> QEMU to fetch TRBs from invalid memory areas. Fix it by properly
> checking the return value of dma_memory_read().

It certainly makes sense to check the return value from
dma_memory_read(), but how can we end up in an infinite loop
if it fails? Either:

(1) there is some combination of data values which allow an
infinite loop, so we can hit those if the DMA access fails and
we use bogus uninitialized data. But then the guest could
maliciously pass us those bad values and create an infinite
loop without a failing DMA access, ie commit 05f43d44e4b
missed a case. If so we need to fix that!

Or (2) there isn't actually an infinite loop possible here,
and we're just tidying up a case which is C undefined
behaviour but in practice unlikely to have effects any
worse than the guest could provoke anyway (ie running up
to the TRB_LINK_LIMIT and stopping). In this case the
commit message here is a bit misleading.

thanks
-- PMM

Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[Thomas Huth]

On 02/08/2022 16.09, Peter Maydell wrote:
> On Tue, 2 Aug 2022 at 14:53, Thomas Huth <thuth@redhat.com> wrote:
>>
>> The XHCI code could enter an endless loop in case the guest points
>> QEMU to fetch TRBs from invalid memory areas. Fix it by properly
>> checking the return value of dma_memory_read().
> 
> It certainly makes sense to check the return value from
> dma_memory_read(), but how can we end up in an infinite loop
> if it fails? Either:
> 
> (1) there is some combination of data values which allow an
> infinite loop, so we can hit those if the DMA access fails and
> we use bogus uninitialized data. But then the guest could
> maliciously pass us those bad values and create an infinite
> loop without a failing DMA access, ie commit 05f43d44e4b
> missed a case. If so we need to fix that!

No, as far as I can see, that's not the case.

> Or (2) there isn't actually an infinite loop possible here,
> and we're just tidying up a case which is C undefined
> behaviour but in practice unlikely to have effects any
> worse than the guest could provoke anyway (ie running up
> to the TRB_LINK_LIMIT and stopping). In this case the
> commit message here is a bit misleading.

So from what I understand, this is happening: The guest somehow manages to 
trick QEMU in a state where it reads through a set of TRBs where none is 
marked in a way that could cause the function to return. QEMU then reads all 
memory 'till the end of RAM and then continues to loop through no-mans-land 
since the return value of dma_memory_read() is not checked. Since 
dma_memory_read() was not able to fetch the memory, the "trb" struct does 
not get updated, i.e. continues to contain data that does not cause the 
function to return. Thus the while(1) loop in xhci_ring_chain_length() 
happily runs through the whole 64-bit address space range, failing to read 
memory and thus leaving the loop. Maybe the loop is not really endless if it 
wrap-arounds the 64-bit address space at one point in time and finally finds 
another chunk of memory in the real RAM that causes the loop to exit, but it 
would still certainly take a long time where QEMU is just completely 
unresponsive and busy stepping through the no-mans-land.

Does that make sense to you now? If so, I can respin a v2 with an improved 
patch description if you like.

  Thomas

Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[Peter Maydell]

On Thu, 4 Aug 2022 at 09:00, Thomas Huth <thuth@redhat.com> wrote:
>
> On 02/08/2022 16.09, Peter Maydell wrote:
> > On Tue, 2 Aug 2022 at 14:53, Thomas Huth <thuth@redhat.com> wrote:
> >>
> >> The XHCI code could enter an endless loop in case the guest points
> >> QEMU to fetch TRBs from invalid memory areas. Fix it by properly
> >> checking the return value of dma_memory_read().
> >
> > It certainly makes sense to check the return value from
> > dma_memory_read(), but how can we end up in an infinite loop
> > if it fails? Either:
> >
> > (1) there is some combination of data values which allow an
> > infinite loop, so we can hit those if the DMA access fails and
> > we use bogus uninitialized data. But then the guest could
> > maliciously pass us those bad values and create an infinite
> > loop without a failing DMA access, ie commit 05f43d44e4b
> > missed a case. If so we need to fix that!
>
> No, as far as I can see, that's not the case.
>
> > Or (2) there isn't actually an infinite loop possible here,
> > and we're just tidying up a case which is C undefined
> > behaviour but in practice unlikely to have effects any
> > worse than the guest could provoke anyway (ie running up
> > to the TRB_LINK_LIMIT and stopping). In this case the
> > commit message here is a bit misleading.
>
> So from what I understand, this is happening: The guest somehow manages to
> trick QEMU in a state where it reads through a set of TRBs where none is
> marked in a way that could cause the function to return. QEMU then reads all
> memory 'till the end of RAM and then continues to loop through no-mans-land
> since the return value of dma_memory_read() is not checked.

But the point of TRB_LINK_LIMIT is that regardless of what the
contents of the TRBs are, the loop is not supposed to
be able to continue for more than TRB_LINK_LIMIT iterations,
ie 32 times. In this example case, do we stop after 32 TRBs
(case 2) or not (case 1)?

thanks
-- PMM

Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[Thomas Huth]

On 04/08/2022 10.56, Peter Maydell wrote:
> On Thu, 4 Aug 2022 at 09:00, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 02/08/2022 16.09, Peter Maydell wrote:
>>> On Tue, 2 Aug 2022 at 14:53, Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>> The XHCI code could enter an endless loop in case the guest points
>>>> QEMU to fetch TRBs from invalid memory areas. Fix it by properly
>>>> checking the return value of dma_memory_read().
>>>
>>> It certainly makes sense to check the return value from
>>> dma_memory_read(), but how can we end up in an infinite loop
>>> if it fails? Either:
>>>
>>> (1) there is some combination of data values which allow an
>>> infinite loop, so we can hit those if the DMA access fails and
>>> we use bogus uninitialized data. But then the guest could
>>> maliciously pass us those bad values and create an infinite
>>> loop without a failing DMA access, ie commit 05f43d44e4b
>>> missed a case. If so we need to fix that!
>>
>> No, as far as I can see, that's not the case.
>>
>>> Or (2) there isn't actually an infinite loop possible here,
>>> and we're just tidying up a case which is C undefined
>>> behaviour but in practice unlikely to have effects any
>>> worse than the guest could provoke anyway (ie running up
>>> to the TRB_LINK_LIMIT and stopping). In this case the
>>> commit message here is a bit misleading.
>>
>> So from what I understand, this is happening: The guest somehow manages to
>> trick QEMU in a state where it reads through a set of TRBs where none is
>> marked in a way that could cause the function to return. QEMU then reads all
>> memory 'till the end of RAM and then continues to loop through no-mans-land
>> since the return value of dma_memory_read() is not checked.
> 
> But the point of TRB_LINK_LIMIT is that regardless of what the
> contents of the TRBs are, the loop is not supposed to
> be able to continue for more than TRB_LINK_LIMIT iterations,
> ie 32 times. In this example case, do we stop after 32 TRBs
> (case 2) or not (case 1)?

Oh, wait, I think we were maybe looking at different spots. The problem 
likely does not occur in the xhci_ring_fetch() function
(which you were likely looking at), but only in the xhci_ring_chain_length() 
function (which I was looking at)!
xhci_ring_chain_length() can certainly continue more than 32 times. In 
xhci_ring_chain_length() the TRB_LINK_LIMIT only applies if "type == 
TR_LINK", but the TRBs we're talking about here are *not* of type TR_LINK.

  Thomas

Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[Peter Maydell]

On Thu, 4 Aug 2022 at 11:07, Thomas Huth <thuth@redhat.com> wrote:
>
> On 04/08/2022 10.56, Peter Maydell wrote:
> > But the point of TRB_LINK_LIMIT is that regardless of what the
> > contents of the TRBs are, the loop is not supposed to
> > be able to continue for more than TRB_LINK_LIMIT iterations,
> > ie 32 times. In this example case, do we stop after 32 TRBs
> > (case 2) or not (case 1)?
>
> Oh, wait, I think we were maybe looking at different spots. The problem
> likely does not occur in the xhci_ring_fetch() function
> (which you were likely looking at), but only in the xhci_ring_chain_length()
> function (which I was looking at)!
> xhci_ring_chain_length() can certainly continue more than 32 times. In
> xhci_ring_chain_length() the TRB_LINK_LIMIT only applies if "type ==
> TR_LINK", but the TRBs we're talking about here are *not* of type TR_LINK.

That sounds like we do still have an unbounded-loop problem,
then: there's no limit on the number of consecutive TRBs
we try to read in that function. Maybe we're missing an
error check of some kind (does the spec limit how many
consecutive TRBs there can be somehow?) or else we need
another artificial limit.

thanks
-- PMM

Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[Thomas Huth]

On 04/08/2022 12.17, Peter Maydell wrote:
> On Thu, 4 Aug 2022 at 11:07, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 04/08/2022 10.56, Peter Maydell wrote:
>>> But the point of TRB_LINK_LIMIT is that regardless of what the
>>> contents of the TRBs are, the loop is not supposed to
>>> be able to continue for more than TRB_LINK_LIMIT iterations,
>>> ie 32 times. In this example case, do we stop after 32 TRBs
>>> (case 2) or not (case 1)?
>>
>> Oh, wait, I think we were maybe looking at different spots. The problem
>> likely does not occur in the xhci_ring_fetch() function
>> (which you were likely looking at), but only in the xhci_ring_chain_length()
>> function (which I was looking at)!
>> xhci_ring_chain_length() can certainly continue more than 32 times. In
>> xhci_ring_chain_length() the TRB_LINK_LIMIT only applies if "type ==
>> TR_LINK", but the TRBs we're talking about here are *not* of type TR_LINK.
> 
> That sounds like we do still have an unbounded-loop problem,
> then: there's no limit on the number of consecutive TRBs
> we try to read in that function. Maybe we're missing an
> error check of some kind (does the spec limit how many
> consecutive TRBs there can be somehow?) or else we need
> another artificial limit.

I'm not an XHCI expert at all, but while at least having a quick glance at 
the spec, I did not see any limit there. So I assume that we should enforce 
an artificial limit? What would be a good value for this? Gerd, do you maybe 
have any opinion?

  Thomas

Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[Thomas Huth]

On 04/08/2022 13.43, Thomas Huth wrote:
> On 04/08/2022 12.17, Peter Maydell wrote:
>> On Thu, 4 Aug 2022 at 11:07, Thomas Huth <thuth@redhat.com> wrote:
>>>
>>> On 04/08/2022 10.56, Peter Maydell wrote:
>>>> But the point of TRB_LINK_LIMIT is that regardless of what the
>>>> contents of the TRBs are, the loop is not supposed to
>>>> be able to continue for more than TRB_LINK_LIMIT iterations,
>>>> ie 32 times. In this example case, do we stop after 32 TRBs
>>>> (case 2) or not (case 1)?
>>>
>>> Oh, wait, I think we were maybe looking at different spots. The problem
>>> likely does not occur in the xhci_ring_fetch() function
>>> (which you were likely looking at), but only in the xhci_ring_chain_length()
>>> function (which I was looking at)!
>>> xhci_ring_chain_length() can certainly continue more than 32 times. In
>>> xhci_ring_chain_length() the TRB_LINK_LIMIT only applies if "type ==
>>> TR_LINK", but the TRBs we're talking about here are *not* of type TR_LINK.
>>
>> That sounds like we do still have an unbounded-loop problem,
>> then: there's no limit on the number of consecutive TRBs
>> we try to read in that function. Maybe we're missing an
>> error check of some kind (does the spec limit how many
>> consecutive TRBs there can be somehow?) or else we need
>> another artificial limit.
> 
> I'm not an XHCI expert at all, but while at least having a quick glance at 
> the spec, I did not see any limit there. So I assume that we should enforce 
> an artificial limit? What would be a good value for this? Gerd, do you maybe 
> have any opinion?

Ok, after looking at the spec a little bit longer, I discovered chapter "6 
Data Structures", where they say that Transfer Ring segments should be 
limited to 64kB each. That sounds like a reasonable limit. I'll update my 
patch accordingly.

  Thomas

Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[Gerd Hoffmann]

On Thu, Aug 04, 2022 at 01:43:14PM +0200, Thomas Huth wrote:
> On 04/08/2022 12.17, Peter Maydell wrote:
> > That sounds like we do still have an unbounded-loop problem,
> > then: there's no limit on the number of consecutive TRBs
> > we try to read in that function. Maybe we're missing an
> > error check of some kind (does the spec limit how many
> > consecutive TRBs there can be somehow?) or else we need
> > another artificial limit.
> 
> I'm not an XHCI expert at all, but while at least having a quick glance at
> the spec, I did not see any limit there. So I assume that we should enforce
> an artificial limit? What would be a good value for this? Gerd, do you maybe
> have any opinion?

Hmm, dunno.  Typical workflow is that the driver allocates a page
(or multiple physically contiguous pages) for the TRBs, and when
it reaches the end of the allocation it gets a new block and chains
it using TRB_LINK.

Right now we have a limit for type=TRB_LINK only, having another one for
all TRBs makes sense.  The number of TRBs for a transfer can be quite
large (think usb-storage reads/writes), so don't set that too low, 64k
maybe?

take care,
  Gerd

Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[Thomas Huth]

On 16/08/2022 10.37, Gerd Hoffmann wrote:
> On Thu, Aug 04, 2022 at 01:43:14PM +0200, Thomas Huth wrote:
>> On 04/08/2022 12.17, Peter Maydell wrote:
>>> That sounds like we do still have an unbounded-loop problem,
>>> then: there's no limit on the number of consecutive TRBs
>>> we try to read in that function. Maybe we're missing an
>>> error check of some kind (does the spec limit how many
>>> consecutive TRBs there can be somehow?) or else we need
>>> another artificial limit.
>>
>> I'm not an XHCI expert at all, but while at least having a quick glance at
>> the spec, I did not see any limit there. So I assume that we should enforce
>> an artificial limit? What would be a good value for this? Gerd, do you maybe
>> have any opinion?
> 
> Hmm, dunno.  Typical workflow is that the driver allocates a page
> (or multiple physically contiguous pages) for the TRBs, and when
> it reaches the end of the allocation it gets a new block and chains
> it using TRB_LINK.
> 
> Right now we have a limit for type=TRB_LINK only, having another one for
> all TRBs makes sense.  The number of TRBs for a transfer can be quite
> large (think usb-storage reads/writes), so don't set that too low, 64k
> maybe?

Yes, after some more reading, I found a remark in the spec (in chapter 6) 
saying that each segment should be limited by 64 kb.

I've sent a v2 with that limit check here (it has a slightly different 
subject, so it's hard to relate it to this v1 here):

 
https://patchwork.kernel.org/project/qemu-devel/patch/20220804131300.96368-1-thuth@redhat.com/

  Thomas

Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[Mauro Matteo Cascella]

On Tue, Aug 2, 2022 at 3:48 PM Thomas Huth <thuth@redhat.com> wrote:
>
> The XHCI code could enter an endless loop in case the guest points
> QEMU to fetch TRBs from invalid memory areas. Fix it by properly
> checking the return value of dma_memory_read().
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/646
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/usb/hcd-xhci.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 296cc6c8e6..63d428a444 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -21,6 +21,7 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/timer.h"
> +#include "qemu/log.h"
>  #include "qemu/module.h"
>  #include "qemu/queue.h"
>  #include "migration/vmstate.h"
> @@ -679,8 +680,12 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
>
>      while (1) {
>          TRBType type;
> -        dma_memory_read(xhci->as, ring->dequeue, trb, TRB_SIZE,
> -                        MEMTXATTRS_UNSPECIFIED);
> +        if (dma_memory_read(xhci->as, ring->dequeue, trb, TRB_SIZE,
> +                            MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory access failed!\n",
> +                          __func__);
> +            return 0;
> +        }
>          trb->addr = ring->dequeue;
>          trb->ccs = ring->ccs;
>          le64_to_cpus(&trb->parameter);
> @@ -727,8 +732,12 @@ static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
>
>      while (1) {
>          TRBType type;
> -        dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE,
> -                        MEMTXATTRS_UNSPECIFIED);
> +        if (dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE,
> +                        MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory access failed!\n",
> +                          __func__);
> +            return -length;

Not strictly related to this issue, but what's the point of returning
-length instead of e.g. -1? Apart from that, LGTM. Thank you.

> +        }
>          le64_to_cpus(&trb.parameter);
>          le32_to_cpus(&trb.status);
>          le32_to_cpus(&trb.control);
> --
> 2.31.1
>

-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[Thomas Huth]

On 04/08/2022 10.45, Mauro Matteo Cascella wrote:
> On Tue, Aug 2, 2022 at 3:48 PM Thomas Huth <thuth@redhat.com> wrote:
>>
>> The XHCI code could enter an endless loop in case the guest points
>> QEMU to fetch TRBs from invalid memory areas. Fix it by properly
>> checking the return value of dma_memory_read().
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/646
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/usb/hcd-xhci.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
>> index 296cc6c8e6..63d428a444 100644
>> --- a/hw/usb/hcd-xhci.c
>> +++ b/hw/usb/hcd-xhci.c
>> @@ -21,6 +21,7 @@
>>
>>   #include "qemu/osdep.h"
>>   #include "qemu/timer.h"
>> +#include "qemu/log.h"
>>   #include "qemu/module.h"
>>   #include "qemu/queue.h"
>>   #include "migration/vmstate.h"
>> @@ -679,8 +680,12 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
>>
>>       while (1) {
>>           TRBType type;
>> -        dma_memory_read(xhci->as, ring->dequeue, trb, TRB_SIZE,
>> -                        MEMTXATTRS_UNSPECIFIED);
>> +        if (dma_memory_read(xhci->as, ring->dequeue, trb, TRB_SIZE,
>> +                            MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory access failed!\n",
>> +                          __func__);
>> +            return 0;
>> +        }
>>           trb->addr = ring->dequeue;
>>           trb->ccs = ring->ccs;
>>           le64_to_cpus(&trb->parameter);
>> @@ -727,8 +732,12 @@ static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
>>
>>       while (1) {
>>           TRBType type;
>> -        dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE,
>> -                        MEMTXATTRS_UNSPECIFIED);
>> +        if (dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE,
>> +                        MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory access failed!\n",
>> +                          __func__);
>> +            return -length;
> 
> Not strictly related to this issue, but what's the point of returning
> -length instead of e.g. -1? Apart from that, LGTM. Thank you.

Yeah, that's a little bit weird, but it's also what the other spots in this 
function are doing, so I didn't want to diverge from that.

  Thomas