Bug 506929 - sysfs syscall not wrapped
Summary: sysfs syscall not wrapped
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (other bugs)
Version First Reported In: 3.25.0
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: mcermak
URL:
Keywords:
Depends on:
Blocks: 506971
  Show dependency treegraph
 
Reported: 2025-07-11 22:27 UTC by Mark Wielaard
Modified: 2025-08-04 11:50 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
proposed patch (11.59 KB, patch)
2025-07-28 10:40 UTC, mcermak
Details
proposed patch (11.00 KB, patch)
2025-08-01 12:08 UTC, mcermak
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Wielaard 2025-07-11 22:27:40 UTC
This is a fairly useless and deprecated syscall.

https://www.man7.org/linux/man-pages/man2/sysfs.2.html

LTP has 5 tests for this sysfs01..sysfs05.

Note that the number of arguments depends on the first argument.
For option 2, getting the fsname, there is no way to know how big the buffer needs to be.
Comment 1 mcermak 2025-07-28 10:40:38 UTC
Created attachment 183590 [details]
proposed patch

Attached proposed patch.  Please review.
Comment 2 Mark Wielaard 2025-07-31 12:15:14 UTC
All the syswrap-<arch>-linux.c wiring seems correct.

Removing the mips specific wrapper seems correct too.

> +PRE(sys_sysfs)                                                               
> +{                                                                            
> +   FUSE_COMPATIBLE_MAY_BLOCK();                                              
> +   switch (ARG1) {                                                           
> +      case 1:                                                                
> +         PRINT("sys_sysfs ( %lu, %lu )", ARG1, ARG2);                        
> +         PRE_REG_READ2(long, "sysfs", int, flags, const void *, path);       
> +         break;

I think your want a PRE_MEM_RASCIIZ("sysfs(path)", ARG2) here to check path points to a defined string.

> +      case 2:                                                                
> +         PRINT("sys_sysfs ( %lu, %lu, %#" FMT_REGWORD "x )",                 
> +               ARG1, ARG2, ARG3);                                            
> +         PRE_REG_READ3(long, "sysfs", int, flags, int, desc, void *, path);  
> +         break;

As you note below we don't know how big the path is or should be.
But at least one byte should be writable. So maybe add
PRE_MEM_WRITE("sysfs(path)", ARG3, 1) ?

> +      case 3:                                                                
> +         PRINT("sys_sysfs ( %lu )", ARG1);                                   
> +         PRE_REG_READ1(long, "sysfs", int, flags);                           
> +         break;                                                              
> +      default:                                                               
> +         if (VG_(clo_verbosity) >= 1) {                                      
> +            VG_(message)(Vg_DebugMsg,                                        
> +               "WARNING: unhandled sysfs option %lu\n", ARG1);               
> +         }                                                                   
> +         break;                                                              
> +   }                                                                         
> +}                                                                            

Ack.

> +vki_size_t my_strlen(const char *s);                                         
> +vki_size_t my_strlen(const char *s) {                                        
> +    vki_size_t len = 0;                                                      
> +    while (s[len] != '\0') {                                                 
> +        len++;                                                               
> +    }                                                                        
> +    return len;                                                              
> +}                                                                            

We already have VG_(strlen) see include/pub_tool_libcbase.h for some "standalone libc stuff".

> +POST(sys_sysfs)                                                              
> +{                                                                            
> +   if (ARG1 == 2) {                                                          
> +      // For option 2, getting the fsname, there is no way to know how big the buffer needs to be.                                                          
> +      POST_MEM_WRITE(ARG3, my_strlen((void *)ARG3));                         
> +   }                                                                         
> +}                                                                            
> +                                                                             

Right. This should work (use VG_(strlen) here) because the POST handler is only called when the syscall succeeds.
So marking that range of memory as being defined should be fine. Otherwise the syscall would have failed with EFAULT.

> diff --git a/include/vki/vki-scnums-32bit-linux.h b/include/vki/vki-scnums-32\
bit-linux.h                                                                     
> index f276ddaed..9fa6b6183 100644                                             
> --- a/include/vki/vki-scnums-32bit-linux.h                                    
> +++ b/include/vki/vki-scnums-32bit-linux.h                                    
> @@ -25,6 +25,7 @@                                                             
>  // Derived from the __BITS_PER_LONG == 32 sections in                        
>  // linux-5.2/include/uapi/asm-generic/unistd.h                               
>                                                                               
> +#define __NR_sysfs   135                                                     
>  #define __NR_clock_gettime64 403                                             
>  #define __NR_clock_settime64 404                                             
>  #define __NR_clock_adjtime64 405                                             

I think this bit is wrong. It looks like __NR_sysfs is already defined for all 32bit linux arches in their include/vki/vki-scnums-<arch>-linux.h file. So redefining it in vki-scnums-32bit-linux.h will clash on 32bit arches.
Comment 3 mcermak 2025-08-01 12:08:05 UTC
Created attachment 183708 [details]
proposed patch

Got it.  Thank you for the review!  Updated (and tested) patch attached.
Comment 4 Mark Wielaard 2025-08-01 14:11:33 UTC
Looks great. Pushed as:

commit 34dff50cf304d1d4f0d5a1ad3f55f8b3d85ae701
Author: Martin Cermak <mcermak@redhat.com>
Date:   Fri Aug 1 14:04:24 2025 +0200

    Wrap linux specific syscall sysfs
    
    The sysfs syscall is deprecated, but in some cases it may still
    be used.  The Linux Test Project covers it.
    
    The (obsolete) sysfs() system call returns information about the
    filesystem types currently present in the kernel.  The specific
    form of the sysfs() call and the information returned depends on
    the option in effect:
    
    1  Translate the filesystem identifier string fsname into a
       filesystem type index.
    
    2  Translate the filesystem type index fs_index into a null-
       terminated filesystem identifier string.  This string will be
       written to the buffer pointed to by buf.  Make sure that buf
       has enough space to accept the string.
    
    3  Return the total number of filesystem types currently present
       in the kernel.
    
    Declare a sys_sysfs wrapper in priv_syswrap-linux.h and hook it
    for {amd64,arm,mips32,mips64,ppc32,ppc64,s390x,x86}-linux
    using LINXY with PRE and POST handler in syswrap-linux.c
    
    https://bugs.kde.org/show_bug.cgi?id=506929