On Thu, Aug 31, 2023 at 6:57 PM Rob Landley <r...@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);
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
