linux-kernel - Re: [PATCH 15/22] hwmon: applesmc: fix format string overflow

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]

Date:   Fri, 14 Jul 2017 07:06:54 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Arnd Bergmann <arnd@...db.de>, linux-kernel@...r.kernel.org,
        Henrik Rydberg <rydberg@...math.org>,
        Jean Delvare <jdelvare@...e.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        akpm@...ux-foundation.org, netdev@...r.kernel.org,
        "David S . Miller" <davem@...emloft.net>,
        "James E . J . Bottomley" <jejb@...ux.vnet.ibm.com>,
        "Martin K . Petersen" <martin.petersen@...cle.com>,
        linux-scsi@...r.kernel.org, x86@...nel.org,
        linux-hwmon@...r.kernel.org
Subject: Re: [PATCH 15/22] hwmon: applesmc: fix format string overflow

On 07/14/2017 05:07 AM, Arnd Bergmann wrote:
> gcc-7 warns that the key might exceed five bytes for lage index
> values:
> 
> drivers/hwmon/applesmc.c: In function 'applesmc_show_fan_position':
> drivers/hwmon/applesmc.c:906:18: error: '%d' directive writing between 1 and 5 bytes into a region of size 4 [-Werror=format-overflow=]
> sprintf(newkey, FAN_ID_FMT, to_index(attr));
>                  ^~~~~~~
> drivers/hwmon/applesmc.c:906:18: note: directive argument in the range [0, 65535]
> drivers/hwmon/applesmc.c:906:2: note: 'sprintf' output between 5 and 9 bytes into a destination of size 5
> 
> As the key is required to be four characters plus trailing zero,
> we know that the index has to be small here. I'm using snprintf()
> to avoid the warning. This would truncate the string instead of
> overflowing.
> 
> Signed-off-by: Arnd Bergmann <arnd@...db.de>

I submitted a more comprehensive patch a couple of days ago. There are other similar
sprintf() calls in the driver which gcc doesn't report.

Guenter

> ---
>   drivers/hwmon/applesmc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 0af7fd311979..515163b9a89f 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -903,7 +903,7 @@ static ssize_t applesmc_show_fan_position(struct device *dev,
>   	char newkey[5];
>   	u8 buffer[17];
>   
> -	sprintf(newkey, FAN_ID_FMT, to_index(attr));
> +	snprintf(newkey, sizeof(newkey), FAN_ID_FMT, to_index(attr));
>   
>   	ret = applesmc_read_key(newkey, buffer, 16);
>   	buffer[16] = 0;
>