The s390-pci-bus.c code, Coverity complains about a possible overflow
because get_table_index() can return -1 if the ett value passed in is
not one of the three permitted ZPCI_ETT_PT, ZPCI_ETT_ST, ZPCI_ETT_RT,
but the caller in table_translate() doesn't check this and instead
uses the return value directly in a calculation of the guest address
to read from.

In fact this case cannot happen, because:
 * get_table_index() is called only from table_translate()
 * the only caller of table_translate() loops through the ett values
   in the order RT, ST, PT until table_translate() returns 0
 * table_translate() will return 0 for the error cases and when
   translate_iscomplete() returns true
 * translate_iscomplete() is always true for ZPCI_ETT_PT

So table_translate() is always called with a valid ett value.

Instead of having the various functions called from table_translate()
return a default or dummy value when the ett argument is out of range,
use g_assert_not_reached() to indicate that this is impossible.

Coverity: CID 1547609
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Disclaimer: only tested with 'make check/make check-functional'

 hw/s390x/s390-pci-bus.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index e6aa44531f6..f87d2748b63 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -384,9 +384,9 @@ static uint64_t get_table_index(uint64_t iova, int8_t ett)
         return calc_sx(iova);
     case ZPCI_ETT_RT:
         return calc_rtx(iova);
+    default:
+        g_assert_not_reached();
     }
-
-    return -1;
 }
 
 static bool entry_isvalid(uint64_t entry, int8_t ett)
@@ -397,22 +397,24 @@ static bool entry_isvalid(uint64_t entry, int8_t ett)
     case ZPCI_ETT_ST:
     case ZPCI_ETT_RT:
         return rt_entry_isvalid(entry);
+    default:
+        g_assert_not_reached();
     }
-
-    return false;
 }
 
 /* Return true if address translation is done */
 static bool translate_iscomplete(uint64_t entry, int8_t ett)
 {
     switch (ett) {
-    case 0:
+    case ZPCI_ETT_ST:
         return (entry & ZPCI_TABLE_FC) ? true : false;
-    case 1:
+    case ZPCI_ETT_RT:
         return false;
+    case ZPCI_ETT_PT:
+        return true;
+    default:
+        g_assert_not_reached();
     }
-
-    return true;
 }
 
 static uint64_t get_frame_size(int8_t ett)
@@ -424,9 +426,9 @@ static uint64_t get_frame_size(int8_t ett)
         return 1ULL << 20;
     case ZPCI_ETT_RT:
         return 1ULL << 31;
+    default:
+        g_assert_not_reached();
     }
-
-    return 0;
 }
 
 static uint64_t get_next_table_origin(uint64_t entry, int8_t ett)
@@ -438,9 +440,9 @@ static uint64_t get_next_table_origin(uint64_t entry, int8_t ett)
         return get_st_pto(entry);
     case ZPCI_ETT_RT:
         return get_rt_sto(entry);
+    default:
+        g_assert_not_reached();
     }
-
-    return 0;
 }
 
 /**
-- 
2.43.0

On 7/10/25 12:15 PM, Peter Maydell wrote:
> The s390-pci-bus.c code, Coverity complains about a possible overflow
> because get_table_index() can return -1 if the ett value passed in is
> not one of the three permitted ZPCI_ETT_PT, ZPCI_ETT_ST, ZPCI_ETT_RT,
> but the caller in table_translate() doesn't check this and instead
> uses the return value directly in a calculation of the guest address
> to read from.
> 
> In fact this case cannot happen, because:
>  * get_table_index() is called only from table_translate()
>  * the only caller of table_translate() loops through the ett values
>    in the order RT, ST, PT until table_translate() returns 0
>  * table_translate() will return 0 for the error cases and when
>    translate_iscomplete() returns true
>  * translate_iscomplete() is always true for ZPCI_ETT_PT
> 
> So table_translate() is always called with a valid ett value.
> 
> Instead of having the various functions called from table_translate()
> return a default or dummy value when the ett argument is out of range,
> use g_assert_not_reached() to indicate that this is impossible.
> 
> Coverity: CID 1547609
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Disclaimer: only tested with 'make check/make check-functional'

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Also to sanity check I ran various tests with s390x guests and a few different PCI passthrough devices using a guest IOMMU to drive table_translate frequently.

Hi Matthew,

On 10/7/25 23:20, Matthew Rosato wrote:
> On 7/10/25 12:15 PM, Peter Maydell wrote:
>> The s390-pci-bus.c code, Coverity complains about a possible overflow
>> because get_table_index() can return -1 if the ett value passed in is
>> not one of the three permitted ZPCI_ETT_PT, ZPCI_ETT_ST, ZPCI_ETT_RT,
>> but the caller in table_translate() doesn't check this and instead
>> uses the return value directly in a calculation of the guest address
>> to read from.
>>
>> In fact this case cannot happen, because:
>>   * get_table_index() is called only from table_translate()
>>   * the only caller of table_translate() loops through the ett values
>>     in the order RT, ST, PT until table_translate() returns 0
>>   * table_translate() will return 0 for the error cases and when
>>     translate_iscomplete() returns true
>>   * translate_iscomplete() is always true for ZPCI_ETT_PT
>>
>> So table_translate() is always called with a valid ett value.
>>
>> Instead of having the various functions called from table_translate()
>> return a default or dummy value when the ett argument is out of range,
>> use g_assert_not_reached() to indicate that this is impossible.
>>
>> Coverity: CID 1547609
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Disclaimer: only tested with 'make check/make check-functional'
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> Also to sanity check I ran various tests with s390x guests and a few different PCI passthrough devices using a guest IOMMU to drive table_translate frequently.

Does that mean we can include your Tested-by: tag?

On Thu, 10 Jul 2025 17:15:52 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> The s390-pci-bus.c code, Coverity complains about a possible overflow
> because get_table_index() can return -1 if the ett value passed in is
> not one of the three permitted ZPCI_ETT_PT, ZPCI_ETT_ST, ZPCI_ETT_RT,
> but the caller in table_translate() doesn't check this and instead
> uses the return value directly in a calculation of the guest address
> to read from.
> 
> In fact this case cannot happen, because:
>  * get_table_index() is called only from table_translate()
>  * the only caller of table_translate() loops through the ett values
>    in the order RT, ST, PT until table_translate() returns 0
>  * table_translate() will return 0 for the error cases and when
>    translate_iscomplete() returns true
>  * translate_iscomplete() is always true for ZPCI_ETT_PT
> 
> So table_translate() is always called with a valid ett value.
> 
> Instead of having the various functions called from table_translate()
> return a default or dummy value when the ett argument is out of range,
> use g_assert_not_reached() to indicate that this is impossible.
> 
> Coverity: CID 1547609
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>