[Toybox] [PATCH] devmem: Fix 8 byte wide writes

[Rob Landley]

On 8/31/23 02:00, Michael Shavit wrote:
> Replace the left-shift method of computing the maximum allowed write as
> it may exibit undefined behavior.
> Add and use an unsigned version of atolx to support writing any 64bit
> value.
> 
> * When bytes equals 8, devmem will left-shift an unsigned long long by
>   64 bits which is undefined behavior if sizeof(unsigned long long) is
>   64.
> * In addition, atolx_range's 'high' parameter is a signed long long and
>   will therefore interpret a high value of ULLONG_MAX as -1.
> * Finally, atolx relies on strtoll under the hood, which will reject
>   values beyond LLONG_MAX.

Could you send me the test case you hit that motivated this change? (I haven't
got a tests/devmem.test yet and am not 100% sure how I'd even set that up in
mkroot under qemu, but I should work out something...)

> ---
> 
>  lib/lib.c           | 39 +++++++++++++++++++++++++++++++++++++++
>  lib/lib.h           |  1 +
>  toys/other/devmem.c | 17 +++++++++++++----
>  3 files changed, 53 insertions(+), 4 deletions(-)

More than doubling the size of the code (47 lines total before, you added 49)
and copying a library function into a command (with duplicate suffix list) to
fix a problem that might just be "use sscanf("%llu", &blah) instead".

Are you using the suffixes? The easy way to get full unsigned range is to drop
the suffixes. (And it documents the 0x prefix so strtoull rather than sscanf...)

Is sizeof(unsigned long long) ever _not_ 8?

I take it you're still ok with the decimal output being signed instead of
unsigned?

How does this look (untested):

 devmem.c |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

--- a/toys/other/devmem.c
+++ b/toys/other/devmem.c
@@ -17,11 +17,20 @@ config DEVMEM
 #define FOR_devmem
 #include "toys.h"
 
+unsigned long long atollu(char *str)
+{
+  char *end = str;
+  unsigned long long llu = strtoul(str, &end, 0);
+
+  if (*end) error_exit("bad %s", str);
+
+  return llu;
+}
+
 void devmem_main(void)
 {
   int writing = toys.optc == 3, page_size = sysconf(_SC_PAGESIZE), bytes = 4,fd;
-  unsigned long long data = 0, map_off, map_len;
-  unsigned long addr = atolx(*toys.optargs);
+  unsigned long long data = 0, map_off, map_len, addr = atollu(*toys.optargs);
   void *map, *p;
 
   // WIDTH?
@@ -34,13 +43,14 @@ void devmem_main(void)
   }
 
   // DATA? Report out of range values as errors rather than truncating.
-  if (writing) data = atolx_range(toys.optargs[2], 0, (1ULL<<(8*bytes))-1);
+  if (writing && (data = atollu(toys.optargs[2]))>(~0LL)>>(64-8*bytes))
+    error_exit("%llx>%d bytes", data, bytes);
 
   // Map in just enough.
   if (CFG_TOYBOX_FORK) {
     fd = xopen("/dev/mem", (writing ? O_RDWR : O_RDONLY) | O_SYNC);
 
-    map_off = addr & ~(page_size - 1);
+    map_off = addr & ~(page_size - 1ULL);
     map_len = (addr+bytes-map_off);
     map = xmmap(0, map_len, writing ? PROT_WRITE : PROT_READ, MAP_SHARED, fd,
         map_off);