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

[Michael Shavit]

On Thu, Aug 31, 2023 at 6:57 PM Rob Landley <rob at landley.net> wrote:
>
> 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...)

I ran something like this:
# devmem 0x000000008b300000 8 0x8FFFFFFFFFFFFFFF
# devmem 0x000000008b300000 8
0x8fffffffffffffff

# devmem 0x000000008b300000 8 500k
# devmem 0x000000008b300000 8
0x0000000007d000

# devmem 0x000000008b300000 8 0xFFFFFFFFFFFFFFFF
# devmem 0x000000008b300000 8
0xffffffffffffffff

# devmem 0x000000008b300000 4 0xFFFFFFFF
# devmem 0x000000008b300000 4
0xffffffff

# devmem 0x000000008b300000  4 0x1FFFFFFFF
devmem: data: 8589934591 exceeds write width: 4

>
> > ---
> >
> >  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...)

I don't, but problem is I don't know if other users of devmem have
come to expect and depend on this behavior. Are we ok potentially
breaking compatibility for a bug fix?

> Is sizeof(unsigned long long) ever _not_ 8?

IIUC, technically the C spec only guarantees that it's at least 8. But
anyways, the intent of using sizeof() was to make sure any updates of
the data/max_value types would propagate to the check.

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

What do you mean, isn't %llx unsigned?

>
> How does this look (untested):

I can test this out in a bit :) .

>
>  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))

Shouldn't it be (~0ULL)? I think (~0LL) would make this an arithmetic
shift instead of a logical shift.

> +    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);