The current check for duplicated sections in a payload is not effective.  Such
check is done inside a loop that iterates over the sections names, it's
logically impossible for the bitmap to be set more than once.

The usage of a bitmap in check_patching_sections() has been replaced with a
boolean, since the function just cares that at least one of the special
sections is present.

No functional change intended, as the check was useless.

Fixes: 29f4ab0b0a4f ('xsplice: Implement support for applying/reverting/replacing patches.')
Fixes: 76b3d4098a92 ('livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/common/livepatch.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index d93a556bcda2..df41dcce970a 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -473,7 +473,6 @@ static int check_special_sections(const struct livepatch_elf *elf)
     static const char *const names[] = { ELF_LIVEPATCH_DEPENDS,
                                          ELF_LIVEPATCH_XEN_DEPENDS,
                                          ELF_BUILD_ID_NOTE};
-    DECLARE_BITMAP(found, ARRAY_SIZE(names)) = { 0 };
 
     for ( i = 0; i < ARRAY_SIZE(names); i++ )
     {
@@ -493,13 +492,6 @@ static int check_special_sections(const struct livepatch_elf *elf)
                    elf->name, names[i]);
             return -EINVAL;
         }
-
-        if ( test_and_set_bit(i, found) )
-        {
-            printk(XENLOG_ERR LIVEPATCH "%s: %s was seen more than once\n",
-                   elf->name, names[i]);
-            return -EINVAL;
-        }
     }
 
     return 0;
@@ -517,7 +509,7 @@ static int check_patching_sections(const struct livepatch_elf *elf)
                                          ELF_LIVEPATCH_PREREVERT_HOOK,
                                          ELF_LIVEPATCH_REVERT_HOOK,
                                          ELF_LIVEPATCH_POSTREVERT_HOOK};
-    DECLARE_BITMAP(found, ARRAY_SIZE(names)) = { 0 };
+    bool found = false;
 
     /*
      * The patching sections are optional, but at least one
@@ -544,16 +536,11 @@ static int check_patching_sections(const struct livepatch_elf *elf)
             return -EINVAL;
         }
 
-        if ( test_and_set_bit(i, found) )
-        {
-            printk(XENLOG_ERR LIVEPATCH "%s: %s was seen more than once\n",
-                   elf->name, names[i]);
-            return -EINVAL;
-        }
+        found = true;
     }
 
     /* Checking if at least one section is present. */
-    if ( bitmap_empty(found, ARRAY_SIZE(names)) )
+    if ( !found )
     {
         printk(XENLOG_ERR LIVEPATCH "%s: Nothing to patch. Aborting...\n",
                elf->name);
-- 
2.46.0

On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> The current check for duplicated sections in a payload is not effective.  Such
> check is done inside a loop that iterates over the sections names, it's
> logically impossible for the bitmap to be set more than once.
>
> The usage of a bitmap in check_patching_sections() has been replaced with a
> boolean, since the function just cares that at least one of the special
> sections is present.
>
> No functional change intended, as the check was useless.
>
> Fixes: 29f4ab0b0a4f ('xsplice: Implement support for applying/reverting/replacing patches.')
> Fixes: 76b3d4098a92 ('livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Yes I agree.  This is useless logic.

The only time we could spot such a case is when matching the section
table with the string table.  For this logic, we always only get
whichever answer livepatch_elf_sec_by_name() decides.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> and I'm very
happy to see some atomics disappear.

On 25.09.2024 10:42, Roger Pau Monne wrote:
> The current check for duplicated sections in a payload is not effective.  Such
> check is done inside a loop that iterates over the sections names, it's
> logically impossible for the bitmap to be set more than once.
> 
> The usage of a bitmap in check_patching_sections() has been replaced with a
> boolean, since the function just cares that at least one of the special
> sections is present.
> 
> No functional change intended, as the check was useless.
> 
> Fixes: 29f4ab0b0a4f ('xsplice: Implement support for applying/reverting/replacing patches.')
> Fixes: 76b3d4098a92 ('livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Just to clarify for my eventual picking up for backporting: Despite
the Fixes: tags there's no actual bug being fixed; it's merely code
simplification. So no need to backport.

Jan

On Wed, Sep 25, 2024 at 10:52:06AM +0200, Jan Beulich wrote:
> On 25.09.2024 10:42, Roger Pau Monne wrote:
> > The current check for duplicated sections in a payload is not effective.  Such
> > check is done inside a loop that iterates over the sections names, it's
> > logically impossible for the bitmap to be set more than once.
> > 
> > The usage of a bitmap in check_patching_sections() has been replaced with a
> > boolean, since the function just cares that at least one of the special
> > sections is present.
> > 
> > No functional change intended, as the check was useless.
> > 
> > Fixes: 29f4ab0b0a4f ('xsplice: Implement support for applying/reverting/replacing patches.')
> > Fixes: 76b3d4098a92 ('livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Just to clarify for my eventual picking up for backporting: Despite
> the Fixes: tags there's no actual bug being fixed; it's merely code
> simplification. So no need to backport.

Indeed, no strict bug, just useless code (unless my analysis is wrong).

Thanks, Roger.