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 -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
