Re: [Qemu-devel] [PATCH v5 1/4] firmware: introduce sysfs driver for QEMU's fw_cfg device
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=] > >> _off, _off, ); > >> ^ > > > > 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 ; 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, > >, , > >_off, _off, ); > > Umm, why are you passing 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 ). 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 '' 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Qemu-devel] [PATCH v5 1/4] firmware: introduce sysfs driver for QEMU's fw_cfg device
On 11/24/15 18:38, 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=] >>> _off, _off, ); >>> ^ >> >> 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 ; have PH_ADDR_FMT > defined to either "lli" or "li", then write sscanf(str, "@%"PH_ADDR_FMT > "%n:..., ...), using PH_ADDR_FMT multiple times. > >> ... >> processed = sscanf(str, PH_ADDR_SCAN_FMT, >>, , >>_off, _off, ); > > Umm, why are you passing to more than one sscanf() %? That's > (probably) undefined behavior. > > [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.] > Yes, but this is the kernel, which may or may not follow POSIX semantics. (And may or may not curse at POSIX in the process, either way! :)) Laszlo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Qemu-devel] [PATCH v5 1/4] firmware: introduce sysfs driver for QEMU's fw_cfg device
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=] >> _off, _off, ); >> ^ > > 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 ; have PH_ADDR_FMT defined to either "lli" or "li", then write sscanf(str, "@%"PH_ADDR_FMT "%n:..., ...), using PH_ADDR_FMT multiple times. > ... > processed = sscanf(str, PH_ADDR_SCAN_FMT, >, , >_off, _off, ); Umm, why are you passing to more than one sscanf() %? That's (probably) undefined behavior. [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.] -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v5 1/4] firmware: introduce sysfs driver for QEMU's fw_cfg device
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=] >> _off, _off, ); >> ^ > > 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 ; have PH_ADDR_FMT defined to either "lli" or "li", then write sscanf(str, "@%"PH_ADDR_FMT "%n:..., ...), using PH_ADDR_FMT multiple times. > ... > processed = sscanf(str, PH_ADDR_SCAN_FMT, >, , >_off, _off, ); Umm, why are you passing to more than one sscanf() %? That's (probably) undefined behavior. [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.] -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v5 1/4] firmware: introduce sysfs driver for QEMU's fw_cfg device
On 11/24/15 18:38, 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=] >>> _off, _off, ); >>> ^ >> >> 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 ; have PH_ADDR_FMT > defined to either "lli" or "li", then write sscanf(str, "@%"PH_ADDR_FMT > "%n:..., ...), using PH_ADDR_FMT multiple times. > >> ... >> processed = sscanf(str, PH_ADDR_SCAN_FMT, >>, , >>_off, _off, ); > > Umm, why are you passing to more than one sscanf() %? That's > (probably) undefined behavior. > > [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.] > Yes, but this is the kernel, which may or may not follow POSIX semantics. (And may or may not curse at POSIX in the process, either way! :)) Laszlo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Qemu-devel] [PATCH v5 1/4] firmware: introduce sysfs driver for QEMU's fw_cfg device
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=] > >> _off, _off, ); > >> ^ > > > > 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 ; 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, > >, , > >_off, _off, ); > > Umm, why are you passing 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 ). 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 '' 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
