[v2] hw/misc: Fix arith overflow in NPCM7XX PWM module

Commit Message

Denis V. Lunev" via Jan. 27, 2021, 1:11 a.m. UTC

Fix potential overflow problem when calculating pwm_duty.
1. Ensure p->cmr and p->cnr to be from [0,65535], according to the
   hardware specification.
2. Changed duty to uint32_t. However, since MAX_DUTY * (p->cmr+1)
   can excceed UINT32_MAX, we convert them to uint64_t in computation
   and converted them back to uint32_t.
   (duty is guaranteed to be <= MAX_DUTY so it won't overflow.)

Fixes: CID 1442342
Suggested-by: Peter Maydell <[e-mail]>
Reviewed-by: Doug Evans <[e-mail]>
Signed-off-by: Hao Wu <[e-mail]>
---
 hw/misc/npcm7xx_pwm.c          | 23 +++++++++++++++++++----
 tests/qtest/npcm7xx_pwm-test.c |  4 ++--
 2 files changed, 21 insertions(+), 6 deletions(-)

Comments

Peter Maydell Jan. 28, 2021, 12:11 p.m. UTC | #1

On Wed, 27 Jan 2021 at 01:11, Hao Wu <[e-mail]> wrote:
>
> Fix potential overflow problem when calculating pwm_duty.
> 1. Ensure p->cmr and p->cnr to be from [0,65535], according to the
>    hardware specification.
> 2. Changed duty to uint32_t. However, since MAX_DUTY * (p->cmr+1)
>    can excceed UINT32_MAX, we convert them to uint64_t in computation
>    and converted them back to uint32_t.
>    (duty is guaranteed to be <= MAX_DUTY so it won't overflow.)
>
> Fixes: CID 1442342
> Suggested-by: Peter Maydell <[e-mail]>
> Reviewed-by: Doug Evans <[e-mail]>
> Signed-off-by: Hao Wu <[e-mail]>
> ---



Applied to target-arm.next, thanks.

-- PMM

Patch

diff --git a/hw/misc/npcm7xx_pwm.c b/hw/misc/npcm7xx_pwm.c
index e99e3cc7ef..dabcb6c0f9 100644
--- a/hw/misc/npcm7xx_pwm.c
+++ b/hw/misc/npcm7xx_pwm.c
@@ -58,6 +58,9 @@  REG32(NPCM7XX_PWM_PWDR3, 0x50);
 #define NPCM7XX_CH_INV              BIT(2)
 #define NPCM7XX_CH_MOD              BIT(3)
 
+#define NPCM7XX_MAX_CMR             65535
+#define NPCM7XX_MAX_CNR             65535
+
 /* Offset of each PWM channel's prescaler in the PPR register. */
 static const int npcm7xx_ppr_base[] = { 0, 0, 8, 8 };
 /* Offset of each PWM channel's clock selector in the CSR register. */
@@ -96,7 +99,7 @@  static uint32_t npcm7xx_pwm_calculate_freq(NPCM7xxPWM *p)
 
 static uint32_t npcm7xx_pwm_calculate_duty(NPCM7xxPWM *p)
 {
-    uint64_t duty;
+    uint32_t duty;
 
     if (p->running) {
         if (p->cnr == 0) {
@@ -104,7 +107,7 @@  static uint32_t npcm7xx_pwm_calculate_duty(NPCM7xxPWM *p)
         } else if (p->cmr >= p->cnr) {
             duty = NPCM7XX_PWM_MAX_DUTY;
         } else {
-            duty = NPCM7XX_PWM_MAX_DUTY * (p->cmr + 1) / (p->cnr + 1);
+            duty = (uint64_t)NPCM7XX_PWM_MAX_DUTY * (p->cmr + 1) / (p->cnr + 1);
         }
     } else {
         duty = 0;
@@ -357,7 +360,13 @@  static void npcm7xx_pwm_write(void *opaque, hwaddr offset,
     case A_NPCM7XX_PWM_CNR2:
     case A_NPCM7XX_PWM_CNR3:
         p = &s->pwm[npcm7xx_cnr_index(offset)];
-        p->cnr = value;
+        if (value > NPCM7XX_MAX_CNR) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: invalid cnr value: %u", __func__, value);
+            p->cnr = NPCM7XX_MAX_CNR;
+        } else {
+            p->cnr = value;
+        }
         npcm7xx_pwm_update_output(p);
         break;
 
@@ -366,7 +375,13 @@  static void npcm7xx_pwm_write(void *opaque, hwaddr offset,
     case A_NPCM7XX_PWM_CMR2:
     case A_NPCM7XX_PWM_CMR3:
         p = &s->pwm[npcm7xx_cmr_index(offset)];
-        p->cmr = value;
+        if (value > NPCM7XX_MAX_CMR) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: invalid cmr value: %u", __func__, value);
+            p->cmr = NPCM7XX_MAX_CMR;
+        } else {
+            p->cmr = value;
+        }
         npcm7xx_pwm_update_output(p);
         break;
 
diff --git a/tests/qtest/npcm7xx_pwm-test.c b/tests/qtest/npcm7xx_pwm-test.c
index 63557d2c06..3d82654b81 100644
--- a/tests/qtest/npcm7xx_pwm-test.c
+++ b/tests/qtest/npcm7xx_pwm-test.c
@@ -272,7 +272,7 @@  static uint64_t pwm_compute_freq(QTestState *qts, uint32_t ppr, uint32_t csr,
 
 static uint64_t pwm_compute_duty(uint32_t cnr, uint32_t cmr, bool inverted)
 {
-    uint64_t duty;
+    uint32_t duty;
 
     if (cnr == 0) {
         /* PWM is stopped. */
@@ -280,7 +280,7 @@  static uint64_t pwm_compute_duty(uint32_t cnr, uint32_t cmr, bool inverted)
     } else if (cmr >= cnr) {
         duty = MAX_DUTY;
     } else {
-        duty = MAX_DUTY * (cmr + 1) / (cnr + 1);
+        duty = (uint64_t)MAX_DUTY * (cmr + 1) / (cnr + 1);
     }
 
     if (inverted) {