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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151124180953.GB22627@HEDWIG.INI.CMU.EDU>
Date:	Tue, 24 Nov 2015 13:09:53 -0500
From:	"Gabriel L. Somlo" <somlo@....edu>
To:	Eric Blake <eblake@...hat.com>
Cc:	kbuild test robot <lkp@...el.com>, mark.rutland@....com,
	peter.maydell@...aro.org, mst@...hat.com, stefanha@...il.com,
	qemu-devel@...gnu.org, eric@...olt.net, kraxel@...hat.com,
	linux-api@...r.kernel.org, pawel.moll@....com, zajec5@...il.com,
	galak@...eaurora.org, rmk+kernel@....linux.org.uk,
	lersek@...hat.com, hanjun.guo@...aro.org,
	devicetree@...r.kernel.org, arnd@...db.de,
	ijc+devicetree@...lion.org.uk, jordan.l.justen@...el.com,
	agross@...eaurora.org, leif.lindholm@...aro.org,
	robh+dt@...nel.org, ard.biesheuvel@...aro.org,
	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
	luto@...capital.net, kbuild-all@...org, sudeep.holla@....com,
	pbonzini@...hat.com, revol@...e.fr
Subject: 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=]
> >>           &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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/