From ee7b71dbc919439b1db6352bcd95f121127b42dd Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Fri, 12 May 2023 17:10:14 -0400 Subject: [PATCH] Fix undefined behavior in spa_sync_props() 8eae2d214cfa53862833eeeda9a5c1e9d5ded47d caused Coverity to begin complaining about "Improper use of negative value" in two places in spa_sync_props() because Coverity correctly inferred from `prop == ZPOOL_PROP_INVAL` that prop could be -1 while both zpool_prop_to_name() and zpool_prop_get_type() use it an array index, which is undefined behavior. Assuming that the system does not panic from an attempt to read invalid memory, the case statement for ZPOOL_PROP_INVAL will ensure that only user properties will reach this code when prop is ZPOOL_PROP_INVAL, such that execution will continue safely. However, if we are unlucky enough to read invalid memory, then the system will panic. This issue predates the patch that caused coverity to begin complaining. Thankfully, our userland tools do not pass nonsense to us, so this bug should not be triggered unless a future userland tool attempts to set a property that we do not understand. Reported-by: Coverity (CID-1561129) Reported-by: Coverity (CID-1561130) Reviewed-by: Brian Behlendorf Reviewed-by: George Amanakis Signed-off-by: Richard Yao Closes #14860 --- module/zfs/spa.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/module/zfs/spa.c b/module/zfs/spa.c index 163961702..1ca114783 100644 --- a/module/zfs/spa.c +++ b/module/zfs/spa.c @@ -8942,12 +8942,12 @@ spa_sync_props(void *arg, dmu_tx_t *tx) } /* normalize the property name */ - propname = zpool_prop_to_name(prop); - proptype = zpool_prop_get_type(prop); - if (prop == ZPOOL_PROP_INVAL && - zfs_prop_user(elemname)) { + if (prop == ZPOOL_PROP_INVAL) { propname = elemname; proptype = PROP_TYPE_STRING; + } else { + propname = zpool_prop_to_name(prop); + proptype = zpool_prop_get_type(prop); } if (nvpair_type(elem) == DATA_TYPE_STRING) {