Subject: Re: [Qemu-devel] [PATCH v5 1/4] firmware: introduce sysfs driver for
 QEMU's fw_cfg device
To: Eric Blake <eblake@redhat.com>, "Gabriel L. Somlo" <somlo@cmu.edu>,
        kbuild test robot <lkp@intel.com>
References: <1448294264-17388-2-git-send-email-somlo@cmu.edu>
 <201511240404.AFpczj7x%fengguang.wu@intel.com>
 <20151124165553.GA22627@HEDWIG.INI.CMU.EDU> <5654A08A.6030002@redhat.com>
Cc: mark.rutland@arm.com, peter.maydell@linaro.org, mst@redhat.com,
        stefanha@gmail.com, qemu-devel@nongnu.org, eric@anholt.net,
        kraxel@redhat.com, linux-api@vger.kernel.org, pawel.moll@arm.com,
        zajec5@gmail.com, galak@codeaurora.org, rmk+kernel@arm.linux.org.uk,
        hanjun.guo@linaro.org, devicetree@vger.kernel.org, arnd@arndb.de,
        ijc+devicetree@hellion.org.uk, jordan.l.justen@intel.com,
        agross@codeaurora.org, leif.lindholm@linaro.org, robh+dt@kernel.org,
        ard.biesheuvel@linaro.org, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org, luto@amacapital.net, kbuild-all@01.org,
        sudeep.holla@arm.com, pbonzini@redhat.com, revol@free.fr
From: Laszlo Ersek <lersek@redhat.com>
Message-ID: <5654A201.2070902@redhat.com>
Date: Tue, 24 Nov 2015 18:44:33 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.3.0
MIME-Version: 1.0
In-Reply-To: <5654A08A.6030002@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2368
Lines: 60

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=]
>>>           &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.
> 
>> ...
>>         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.
> 
> [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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/