[PATCH for-4.19?] libelf: avoid UB in elf_xen_feature_{get,set}()

Jan Beulich posted 1 patch 5 months ago
Failed in applying to current master (apply log)
[PATCH for-4.19?] libelf: avoid UB in elf_xen_feature_{get,set}()
Posted by Jan Beulich 5 months ago
When the left shift amount is up to 31, the shifted quantity wants to be
of unsigned int (or wider) type.

While there also adjust types: get doesn't alter the array and returns a
boolean, while both don't really accept negative "nr". Drop a stray
blank each as well.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Really I wonder why these exist at all; they're effectively test_bit()
and __set_bit() in hypervisor terms, and iirc something like that exists
in the tool stack as well.

--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -445,13 +445,13 @@ struct elf_dom_parms {
     uint64_t virt_kend;
 };
 
-static inline void elf_xen_feature_set(int nr, uint32_t * addr)
+static inline void elf_xen_feature_set(unsigned int nr, uint32_t *addr)
 {
-    addr[nr >> 5] |= 1 << (nr & 31);
+    addr[nr >> 5] |= 1U << (nr & 31);
 }
-static inline int elf_xen_feature_get(int nr, uint32_t * addr)
+static inline bool elf_xen_feature_get(unsigned int nr, const uint32_t *addr)
 {
-    return !!(addr[nr >> 5] & (1 << (nr & 31)));
+    return addr[nr >> 5] & (1U << (nr & 31));
 }
 
 elf_errorstatus elf_xen_parse_features(const char *features,
Re: [PATCH for-4.19?] libelf: avoid UB in elf_xen_feature_{get,set}()
Posted by Andrew Cooper 5 months ago
On 20/06/2024 4:34 pm, Jan Beulich wrote:
> When the left shift amount is up to 31, the shifted quantity wants to be
> of unsigned int (or wider) type.
>
> While there also adjust types: get doesn't alter the array and returns a
> boolean, while both don't really accept negative "nr". Drop a stray
> blank each as well.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

+1 for 4.19.

> ---
> Really I wonder why these exist at all; they're effectively test_bit()
> and __set_bit() in hypervisor terms, and iirc something like that exists
> in the tool stack as well.

The toolstack has tools/libs/ctrl/xc_bitops.h but they're not API
compatible with Xen.

They're long-granular rather than int-granular, have swapped arguments,
and are non-LOCKed.

~Andrew
Re: [PATCH for-4.19?] libelf: avoid UB in elf_xen_feature_{get,set}()
Posted by Oleksii K. 5 months ago
On Thu, 2024-06-20 at 17:07 +0100, Andrew Cooper wrote:
> On 20/06/2024 4:34 pm, Jan Beulich wrote:
> > When the left shift amount is up to 31, the shifted quantity wants
> > to be
> > of unsigned int (or wider) type.
> > 
> > While there also adjust types: get doesn't alter the array and
> > returns a
> > boolean, while both don't really accept negative "nr". Drop a stray
> > blank each as well.
> > 
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> +1 for 4.19.
Release-Acked-By: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii

> 
> > ---
> > Really I wonder why these exist at all; they're effectively
> > test_bit()
> > and __set_bit() in hypervisor terms, and iirc something like that
> > exists
> > in the tool stack as well.
> 
> The toolstack has tools/libs/ctrl/xc_bitops.h but they're not API
> compatible with Xen.
> 
> They're long-granular rather than int-granular, have swapped
> arguments,
> and are non-LOCKed.
> 
> ~Andrew