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

2015-11-24 Thread Eric Blake 
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

2015-11-24 Thread Laszlo Ersek 
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

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

2015-11-24 Thread Gabriel L. Somlo 
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

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

2015-11-24 Thread Gabriel L. Somlo 
On Tue, Nov 24, 2015 at 04:14:50AM +0800, kbuild test robot wrote:
> Hi Gabriel,
> 
> [auto build test WARNING on v4.4-rc2]
> [also build test WARNING on next-20151123]
> [cannot apply to robh/for-next]
> 
> url:
> https://github.com/0day-ci/linux/commits/Gabriel-L-Somlo/SysFS-driver-for-QEMU-fw_cfg-device/20151124-000402
> config: arm-allyesconfig (attached as .config)
> reproduce:
> wget 
> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
>  -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=arm 
> 
> All warnings (new ones prefixed by >>):
> 
>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:

...
phys_addr_t base;
resource_size_t size, ctrl_off, data_off;
...
/* get "@[::]" chunks */
processed = sscanf(str, "@%lli%n:%lli:%lli%n"
   , ,
   _off, _off, );
...

On some architectures, phys_addr_t (and resource_size_t) are u32,
rather than u64. In include/linux/types.h we have:

...
#ifdef CONFIG_PHYS_ADDR_T_64BIT
typedef u64 phys_addr_t;
#else
typedef u32 phys_addr_t;
#endif
...

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
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
...
processed = sscanf(str, PH_ADDR_SCAN_FMT,
   , ,
   _off, _off, );


This should make the warning go away, and be correct. Does that sound
like a valid plan, or is there some other stylistic reason I should
stay away from doing it that way ?

thanks,
--Gabriel

> >> drivers/firmware/qemu_fw_cfg.c:510:7: warning: format '%lli' expects 
> >> argument of type 'long long int *', but argument 5 has type 
> >> 'resource_size_t *' [-Wformat=]
>drivers/firmware/qemu_fw_cfg.c:510:7: warning: format '%lli' expects 
> argument of type 'long long int *', but argument 6 has type 'resource_size_t 
> *' [-Wformat=]
>drivers/firmware/qemu_fw_cfg.c: In function 'fw_cfg_cmdline_get':
> >> drivers/firmware/qemu_fw_cfg.c:563:5: warning: format '%llx' expects 
> >> argument of type 'long long unsigned int', but argument 4 has type 
> >> 'resource_size_t' [-Wformat=]
> fw_cfg_cmdline_dev->resource[0].start);
> ^
>drivers/firmware/qemu_fw_cfg.c:563:5: warning: format '%llx' expects 
> argument of type 'long long unsigned int', but argument 5 has type 
> 'resource_size_t' [-Wformat=]
>drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llx' expects 
> argument of type 'long long unsigned int', but argument 4 has type 
> 'resource_size_t' [-Wformat=]
> fw_cfg_cmdline_dev->resource[2].start);
> ^
>drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llx' expects 
> argument of type 'long long unsigned int', but argument 5 has type 
> 'resource_size_t' [-Wformat=]
> >> drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llu' expects 
> >> argument of type 'long long unsigned int', but argument 6 has type 
> >> 'resource_size_t' [-Wformat=]
>drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llu' expects 
> argument of type 'long long unsigned int', but argument 7 has type 
> 'resource_size_t' [-Wformat=]
> 
> vim +510 drivers/firmware/qemu_fw_cfg.c
> 
>504/* consume "" portion of command line argument */
>505size = memparse(arg, );
>506
>507/* get "@[::]" chunks */
>508processed = sscanf(str, "@%lli%n:%lli:%lli%n",
>509   , ,
>  > 510   _off, _off, );
>511
>512/* sscanf() must process precisely 1 or 3 chunks:
>513 *  is mandatory, optionally followed by 
> 
>514 * and ;
>515 * there must be no extra characters after the last 
> chunk,
>516 * so str[consumed] must be '\0'.
>517 */
>518if (str[consumed] ||
>519(processed != 1 && processed != 3))
>520return -EINVAL;
>521
>522res[0].start = base;
>523res[0].end = base + size - 1;
>524res[0].flags = !strcmp(kp->name, "mmio")

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

2015-11-23 Thread kbuild test robot 
Hi Gabriel,

[auto build test WARNING on v4.4-rc2]
[also build test WARNING on next-20151123]
[cannot apply to robh/for-next]

url:
https://github.com/0day-ci/linux/commits/Gabriel-L-Somlo/SysFS-driver-for-QEMU-fw_cfg-device/20151124-000402
config: arm-allyesconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

   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, );
  ^
>> drivers/firmware/qemu_fw_cfg.c:510:7: warning: format '%lli' expects 
>> argument of type 'long long int *', but argument 5 has type 'resource_size_t 
>> *' [-Wformat=]
   drivers/firmware/qemu_fw_cfg.c:510:7: warning: format '%lli' expects 
argument of type 'long long int *', but argument 6 has type 'resource_size_t *' 
[-Wformat=]
   drivers/firmware/qemu_fw_cfg.c: In function 'fw_cfg_cmdline_get':
>> drivers/firmware/qemu_fw_cfg.c:563:5: warning: format '%llx' expects 
>> argument of type 'long long unsigned int', but argument 4 has type 
>> 'resource_size_t' [-Wformat=]
fw_cfg_cmdline_dev->resource[0].start);
^
   drivers/firmware/qemu_fw_cfg.c:563:5: warning: format '%llx' expects 
argument of type 'long long unsigned int', but argument 5 has type 
'resource_size_t' [-Wformat=]
   drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llx' expects 
argument of type 'long long unsigned int', but argument 4 has type 
'resource_size_t' [-Wformat=]
fw_cfg_cmdline_dev->resource[2].start);
^
   drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llx' expects 
argument of type 'long long unsigned int', but argument 5 has type 
'resource_size_t' [-Wformat=]
>> drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llu' expects 
>> argument of type 'long long unsigned int', but argument 6 has type 
>> 'resource_size_t' [-Wformat=]
   drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llu' expects 
argument of type 'long long unsigned int', but argument 7 has type 
'resource_size_t' [-Wformat=]

vim +510 drivers/firmware/qemu_fw_cfg.c

   504  /* consume "" portion of command line argument */
   505  size = memparse(arg, );
   506  
   507  /* get "@[::]" chunks */
   508  processed = sscanf(str, "@%lli%n:%lli:%lli%n",
   509 , ,
 > 510 _off, _off, );
   511  
   512  /* sscanf() must process precisely 1 or 3 chunks:
   513   *  is mandatory, optionally followed by 
   514   * and ;
   515   * there must be no extra characters after the last chunk,
   516   * so str[consumed] must be '\0'.
   517   */
   518  if (str[consumed] ||
   519  (processed != 1 && processed != 3))
   520  return -EINVAL;
   521  
   522  res[0].start = base;
   523  res[0].end = base + size - 1;
   524  res[0].flags = !strcmp(kp->name, "mmio") ? IORESOURCE_MEM :
   525 IORESOURCE_IO;
   526  
   527  /* insert register offsets, if provided */
   528  if (processed > 1) {
   529  res[1].name = "ctrl";
   530  res[1].start = ctrl_off;
   531  res[1].flags = IORESOURCE_REG;
   532  res[2].name = "data";
   533  res[2].start = data_off;
   534  res[2].flags = IORESOURCE_REG;
   535  }
   536  
   537  /* "processed" happens to nicely match the number of resources
   538   * we need to pass in to this platform device.
   539   */
   540  fw_cfg_cmdline_dev = platform_device_register_simple("fw_cfg",
   541  PLATFORM_DEVID_NONE, res, 
processed);
   542  if (IS_ERR(fw_cfg_cmdline_dev))
   543  return PTR_ERR(fw_cfg_cmdline_dev);
   544  
   545  return 0;
   546  }
   547  
   548  static int fw_cfg_cmdline_get(char *buf, const struct kernel_param *kp)
   549  {
   550  /* stay silent if device was not configured via the command
   551   * line, or if the parameter name (ioport/mmio) doesn't match
   552   * the device setting
   553   */
   554  if (!fw_cfg_cmdline_dev ||
   555  (!strcmp(kp->name, "mmio") ^
   556   (fw_cfg_cmdline_dev->resource[0].flags == IORESOURCE_MEM)))
   557  return 0;
   558  
   559  switch (fw_cfg_cmdline_dev->num_resources) {
   560  case 1:
   561

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

2015-11-23 Thread Gabriel L. Somlo 
From: Gabriel Somlo 

Make fw_cfg entries of type "file" available via sysfs. Entries
are listed under /sys/firmware/qemu_fw_cfg/by_key, in folders
named after each entry's selector key. Filename, selector value,
and size read-only attributes are included for each entry. Also,
a "raw" attribute allows retrieval of the full binary content of
each entry.

The fw_cfg device can be instantiated automatically from ACPI or
the Device Tree, or manually by using a kernel module (or command
line) parameter, with a syntax outlined in the documentation file.

Signed-off-by: Gabriel Somlo 
---
 .../ABI/testing/sysfs-firmware-qemu_fw_cfg |  58 ++
 drivers/firmware/Kconfig   |  19 +
 drivers/firmware/Makefile  |   1 +
 drivers/firmware/qemu_fw_cfg.c | 611 +
 4 files changed, 689 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-qemu_fw_cfg
 create mode 100644 drivers/firmware/qemu_fw_cfg.c

diff --git a/Documentation/ABI/testing/sysfs-firmware-qemu_fw_cfg 
b/Documentation/ABI/testing/sysfs-firmware-qemu_fw_cfg
new file mode 100644
index 000..718fbac
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-qemu_fw_cfg
@@ -0,0 +1,58 @@
+What:  /sys/firmware/qemu_fw_cfg/
+Date:  August 2015
+Contact:   Gabriel Somlo 
+Description:
+   Several different architectures supported by QEMU (x86, arm,
+   sun4*, ppc/mac) are provisioned with a firmware configuration
+   (fw_cfg) device, originally intended as a way for the host to
+   provide configuration data to the guest firmware. Starting
+   with QEMU v2.4, arbitrary fw_cfg file entries may be specified
+   by the user on the command line, which makes fw_cfg additionally
+   useful as an out-of-band, asynchronous mechanism for providing
+   configuration data to the guest userspace.
+
+   The authoritative guest-side hardware interface documentation
+   to the fw_cfg device ca be found in "docs/specs/fw_cfg.txt" in
+   the QEMU source tree.
+
+   === SysFS fw_cfg Interface ===
+
+   The fw_cfg sysfs interface described in this document is only
+   intended to display discoverable blobs (i.e., those registered
+   with the file directory), as there is no way to determine the
+   presence or size of "legacy" blobs (with selector keys between
+   0x0002 and 0x0018) programmatically.
+
+   All fw_cfg information is shown under:
+
+   /sys/firmware/qemu_fw_cfg/
+
+   The only legacy blob displayed is the fw_cfg device revision:
+
+   /sys/firmware/qemu_fw_cfg/rev
+
+   --- Discoverable fw_cfg blobs by selector key ---
+
+   All discoverable blobs listed in the fw_cfg file directory are
+   displayed as entries named after their unique selector key
+   value, e.g.:
+
+   /sys/firmware/qemu_fw_cfg/by_key/32
+   /sys/firmware/qemu_fw_cfg/by_key/33
+   /sys/firmware/qemu_fw_cfg/by_key/34
+   ...
+
+   Each such fw_cfg sysfs entry has the following values exported
+   as attributes:
+
+   name: The 56-byte nul-terminated ASCII string used as the
+ blob's 'file name' in the fw_cfg directory.
+   size: The length of the blob, as given in the fw_cfg
+ directory.
+   key : The value of the blob's selector key as given in the
+ fw_cfg directory. This value is the same as used in
+ the parent directory name.
+   raw : The raw bytes of the blob, obtained by selecting the
+ entry via the control register, and reading a number
+ of bytes equal to the blob size from the data
+ register.
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index cf478fe..3cf920a 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -161,6 +161,25 @@ config RASPBERRYPI_FIRMWARE
  This option enables support for communicating with the firmware on the
  Raspberry Pi.
 
+config FW_CFG_SYSFS
+   tristate "QEMU fw_cfg device support in sysfs"
+   depends on SYSFS
+   default n
+   help
+ Say Y or M here to enable the exporting of the QEMU firmware
+ configuration (fw_cfg) file entries via sysfs. Entries are
+ found under /sys/firmware/fw_cfg when this option is enabled
+ and loaded.
+
+config FW_CFG_SYSFS_CMDLINE
+   bool "QEMU fw_cfg device parameter parsing"
+   depends on FW_CFG_SYSFS