Re: [Qemu-devel] [PATCH v5 1/4] firmware: introduce sysfs driver for QEMU's fw_cfg device

Gabriel L. Somlo
2015-11-24T18:09:53Z

On Tue, Nov 24, 2015 at 10:38:18AM -0700, Eric Blake wrote:
> On 11/24/2015 09:55 AM, Gabriel L. Somlo wrote:
> > On Tue, Nov 24, 2015 at 04:14:50AM +0800, kbuild test robot wrote:
>
> >>
> >> drivers/firmware/qemu_fw_cfg.c: In function 'fw_cfg_cmdline_set':
> >>>> drivers/firmware/qemu_fw_cfg.c:510:7: warning: format '%lli' expects argument of type 'long long int *', but argument 3 has type 'phys_addr_t *' [-Wformat=]
> >> &ctrl_off, &data_off, &consumed);
> >> ^
> >
> > Oh, I think I know why this happened:
> >
>
> >
> > So, I could use u64 instead of phys_addr_t and resource_size_t, and
> > keep "%lli" (or "%Li"), but then I'd have to check if the parsed value
>
> %Li is not POSIX. Don't use it (stick with %lli).
>
> > would overflow a 32-bit address value on arches where phys_addr_t is
> > u32, which would make things a bit more messy and awkward.
> >
> > I'm planning on #ifdef-ing the format string instead:
> >
> > #ifdef CONFIG_PHYS_ADDR_T_64BIT
> > #define PH_ADDR_SCAN_FMT "@%Li%n:%Li:%Li%n"
> > #else
> > #define PH_ADDR_SCAN_FMT "@%li%n:%li:%li%n"
> > #endif
>
> A more typical approach is akin to <inttypes.h>; have PH_ADDR_FMT
> defined to either "lli" or "li", then write sscanf(str, "@%"PH_ADDR_FMT
> "%n:..., ...), using PH_ADDR_FMT multiple times.

That sounds almost like it should be a separate patch against
include/linux/types.h:

diff --git a/include/linux/types.h b/include/linux/types.h
index 70d8500..35be16e 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -160,8 +160,10 @@ typedef unsigned __bitwise__ oom_flags_t;

#ifdef CONFIG_PHYS_ADDR_T_64BIT
typedef u64 phys_addr_t;
+#define __PHYS_ADDR_PREFIX "ll"
#else
typedef u32 phys_addr_t;
+#define __PHYS_ADDR_PREFIX "l"
#endif

typedef phys_addr_t resource_size_t;

But whether it's a good idea for me to detour from fw_cfg/sysfs into
sorting this out with the kernel community right now, I don't know :)

I'll just try to do it inside the fw_cfg sysfs driver for now, see how
that goes...

>
> > ...
> > processed = sscanf(str, PH_ADDR_SCAN_FMT,
> > &base, &consumed,
> > &ctrl_off, &data_off, &consumed);
>
> Umm, why are you passing &consumed to more than one sscanf() %? That's
> (probably) undefined behavior.

Input might end after reading 'base', in which case %n would store the
next character's index in consumed, and evaluate (but otherwise
ignore) the remaining pointer arguments (including the second &consumed).

Or, it might end after reading data_off, then the earlier value of
consumed gets overwritten with the new (past data_off) index. I get to
verify that str[index] is '\0', i.e. that there were no left-over,
unprocessed characters, whether I got one or three items processed by
scanf.

I don't think passing '&consumed' in twice is a problem. I also didn't
cleverly come up with this myself, but rather lifted it from
drivers/virtio/virtio_mmio.c, so at least there's precedent :)

> [In general, sscanf() is a horrid interface to use for parsing integers
> - it has undefined behavior if the input text would trigger integer
> overflow, making it safe to use ONLY on text that you control and can
> guarantee won't overflow. By the time you've figured out if untrusted
> text meets the requirement for safe parsing via sscanf(), you've
> practically already parsed it via safer strtol() and friends.]

Just like (I think) is the case with virtio_mmio, this is an optional
feature to allow specifying a base address, range, and register
offsets for fw_cfg via the insmod (or modprobe) command line, so one
would already have to be root. Also, perfectly well-formated base and
size values could be used to hose the system, which is why virtio_mmio
(and also fw_cfg) leave this feature off by default, and recommend
caution before one would turn it on.

Thanks much,
--Gabriel