From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751390AbdBUGNp (ORCPT ); Tue, 21 Feb 2017 01:13:45 -0500 Received: from mailout4.samsung.com ([203.254.224.34]:40160 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751041AbdBUGMl (ORCPT ); Tue, 21 Feb 2017 01:12:41 -0500 X-AuditID: b6c32a2c-f79b56d0000012f0-bb-58abda4f3a09 Subject: Re: [PATCH 1/2] Fix format overflows To: Jonathan Dieter , linux-kernel@vger.kernel.org Cc: Valentina Manea , Shuah Khan , Peter Senna Tschudin , Greg Kroah-Hartman , "open list:USB OVER IP DRIVER" From: Krzysztof Opasiak Message-id: Date: Tue, 21 Feb 2017 07:12:27 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-version: 1.0 In-reply-to: <20170220205129.18194-1-jdieter@lesbg.com> Content-type: text/plain; charset=iso-8859-2; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprEKsWRmVeSWpSXmKPExsWy7bCmuq7/rdURBgu6NCyaF69ns7h+5zib xeVdc9gsFi1rZbb49raT0WLKy3XsFu8uzWV3YPfYOesuu8emVZ1sHnufyXjsn7uG3ePzJrkA 1igum5TUnMyy1CJ9uwSujJtb/7MUNMpXTJ+xlb2BcZlkFyMnh4SAicS/dwsYIWwxiQv31rN1 MXJxCAksZZR4cuEdG0hCSKCdSWLbdCOYhtdLLrNAFC1nlFjzdQEThPOAUeL1lnYmkCphAV2J z0u2s4PYIgKuEn+mNoGNZRb4CDT22BGgIg4ONgF9iXm7REFqeAXsJGZ++sIKYrMIqEqsurQE bLOoQIzE63l/2SFqBCV+TL7HAmJzCphL7GzYAFbDLGAv8fzuTFYIW15i85q3zCC7JARWsUtc 2bGLBWSXhICsxKYDzBAfuEgsWX6FFcIWlnh1fAs7hC0t8XfpLUaI3mZGiY49z1ggnAmMEtvW HYKqspb4s2oi1GY+id7fT5ggFvBKdLQJQZgeEtNWhEFUO0r8ur2eGRJAXYwSH3YvY5nAKD8L yT+zkPwwC8kPCxiZVzGKpRYU56anFpsWGOoVJ+YWl+al6yXn525iBKcULZ0djPcWeB9iFOBg VOLhtdiyOkKINbGsuDL3EKMEB7OSCO/mm0Ah3pTEyqrUovz4otKc1OJDjNIcLErivFEGEyOE BNITS1KzU1MLUotgskwcnFINjJ0rpx18vVwj4ILYJ4b04pbeVZmd34/JVXw3nzhv2U71mCaf ut+5CwwOl6vyV0x2TDB8FZw//xL/sdeOKywOzJs3w/RMT+G1fCmzNo7YQzOdMw3bCv/GO5te ke80SMrmeWe8o3vjo/c6N5i9GMO5eb55zMtQ3yUtxBm1+M/hi1n/DD/xfmp6r8RSnJFoqMVc VJwIAPXsYZklAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrKIsWRmVeSWpSXmKPExsVy+t9jAV3/W6sjDH6/5rNoXryezeL6neNs Fpd3zWGzWLSsldni29tORospL9exW7y7NJfdgd1j56y77B6bVnWyeex9JuOxf+4ado/Pm+QC WKPcbDJSE1NSixRS85LzUzLz0m2VQkPcdC2UFPISc1NtlSJ0fUOClBTKEnNKgTwjAzTg4Bzg Hqykb5fglnFz63+Wgkb5iukztrI3MC6T7GLk5JAQMJF4veQyC4QtJnHh3nq2LkYuDiGBpYwS O69sY4FwHjFKXHi0hR2kSlhAV+Lzku1gtoiAq8SfqU1QHT2MEtMXbmQFcZgFPjJKfL3zm7mL kYODTUBfYt4uUZAGXgE7iZmfvrCC2CwCqhKrLi1hA7FFBWIkXu5ZxQJRIyjxY/I9MJtTwFxi Z8MGsBpmAWuJZ0ceskLY8hKb17xlnsAoMAtJyywkZbOQlC1gZF7FKJFakFxQnJSea5SXWq5X nJhbXJqXrpecn7uJERxlz6R3MB7e5X6IUYCDUYmHd8ePVRFCrIllxZW5hxglOJiVRHg331wd IcSbklhZlVqUH19UmpNafIjRFOiRicxSosn5wASQVxJvaGJuYm5sYGFuaWlipCTO2zj7WbiQ QHpiSWp2ampBahFMHxMHp1QDY6rsrXuW19q5783U+BqSG2n36Zge8658h8DvfgWbK6Jqjk1b 8J37ohrnzW8uEhE3QmRXbNoSMaPuqoVj96z28GWprOpfxQSOuU+LZlyYfODSTmO5rR92F+Xy P31cff72gW9Wt7MPBX3aO2Gb/9fqwC9nvFewqoReMeqZv4rpd5fJGv+AmhmripRYijMSDbWY i4oTARAaS8zIAgAA X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170221061231epcas5p39be2e28c492c02b1bc23dd071a4cb230 X-Msg-Generator: CA X-Sender-IP: 203.254.230.27 X-Local-Sender: =?UTF-8?B?S3J6eXN6dG9mIE9wYXNpYWsbU1JQT0wtU3lzdGVtIChUUCkb?= =?UTF-8?B?7IK87ISx7KCE7J6QG1NvZnR3YXJlIEVuZ2luZWVy?= X-Global-Sender: =?UTF-8?B?S3J6eXN6dG9mIE9wYXNpYWsbU1JQT0wtU3lzdGVtIChUUCkb?= =?UTF-8?B?U2Ftc3VuZ8KgRWxlY3Ryb25pY3MbU29mdHdhcmUgRW5naW5lZXI=?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjczOTY=?= CMS-TYPE: 105P X-HopCount: 7 X-CMS-RootMailID: 20170220205937epcas3p4e334a658cf4d67f4a7d5a4ae7ce10afc X-RootMTR: 20170220205937epcas3p4e334a658cf4d67f4a7d5a4ae7ce10afc References: <20170220205129.18194-1-jdieter@lesbg.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, W dniu 2017-02-20 o 21:51, Jonathan Dieter pisze: > The usbip userspace tools call sprintf()/snprintf() and don't check for > the return value which can lead the paths to overflow, truncating the > final file in the path. > > More urgently, GCC 7 now warns that these aren't checked with > -Wformat-overflow, and with -Werror enabled in configure.ac, that makes > these tools unbuildable. > > This patch fixes these problems by replacing sprintf() with snprintf() in > one place and adding checks for the return value of snprintf(). > > Reviewed-by: Peter Senna Tschudin > Signed-off-by: Jonathan Dieter > --- > tools/usb/usbip/libsrc/usbip_common.c | 8 +++++++- > tools/usb/usbip/libsrc/usbip_host_common.c | 25 ++++++++++++++++++++----- > 2 files changed, 27 insertions(+), 6 deletions(-) > > diff --git a/tools/usb/usbip/libsrc/usbip_common.c b/tools/usb/usbip/libsrc/usbip_common.c > index ac73710..fc875ee 100644 > --- a/tools/usb/usbip/libsrc/usbip_common.c > +++ b/tools/usb/usbip/libsrc/usbip_common.c > @@ -215,9 +215,15 @@ int read_usb_interface(struct usbip_usb_device *udev, int i, > struct usbip_usb_interface *uinf) > { > char busid[SYSFS_BUS_ID_SIZE]; > + int size; > struct udev_device *sif; > > - sprintf(busid, "%s:%d.%d", udev->busid, udev->bConfigurationValue, i); > + size = snprintf(busid, SYSFS_BUS_ID_SIZE, "%s:%d.%d", why not sizeof(busid)? > + udev->busid, udev->bConfigurationValue, i); > + if (size >= SYSFS_BUS_ID_SIZE) { As above. > + err("busid length %i >= SYSFS_BUS_ID_SIZE", size); > + return -1; > + } > > sif = udev_device_new_from_subsystem_sysname(udev_context, "usb", busid); > if (!sif) { > diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c b/tools/usb/usbip/libsrc/usbip_host_common.c > index 9d41522..690cd49 100644 > --- a/tools/usb/usbip/libsrc/usbip_host_common.c > +++ b/tools/usb/usbip/libsrc/usbip_host_common.c > @@ -40,13 +40,19 @@ struct udev *udev_context; > static int32_t read_attr_usbip_status(struct usbip_usb_device *udev) > { > char status_attr_path[SYSFS_PATH_MAX]; > + int size; > int fd; > int length; > char status; > int value = 0; > > - snprintf(status_attr_path, SYSFS_PATH_MAX, "%s/usbip_status", > - udev->path); > + size = snprintf(status_attr_path, SYSFS_PATH_MAX, "%s/usbip_status", The same here. > + udev->path); > + if (size >= SYSFS_PATH_MAX) { > + err("usbip_status path length %i >= SYSFS_PATH_MAX", size); > + return -1; > + } > + > > fd = open(status_attr_path, O_RDONLY); > if (fd < 0) { > @@ -218,6 +224,7 @@ int usbip_export_device(struct usbip_exported_device *edev, int sockfd) > { > char attr_name[] = "usbip_sockfd"; > char sockfd_attr_path[SYSFS_PATH_MAX]; > + int size; > char sockfd_buff[30]; > int ret; > > @@ -237,10 +244,18 @@ int usbip_export_device(struct usbip_exported_device *edev, int sockfd) > } > > /* only the first interface is true */ > - snprintf(sockfd_attr_path, sizeof(sockfd_attr_path), "%s/%s", > - edev->udev.path, attr_name); > + size = snprintf(sockfd_attr_path, sizeof(sockfd_attr_path), "%s/%s", > + edev->udev.path, attr_name); > + if (size >= SYSFS_PATH_MAX) { hmmm this should be sizeof(sockfd_attr_path) not SYSFS_PATH_MAX > + err("exported device path length %i >= SYSFS_PATH_MAX", size); > + return -1; > + } > > - snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", sockfd); > + size = snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", sockfd); > + if (size >= 30) { Please don't hardcode such values in if. use sizeof() like one line above > + err("socket length %i >= 30", size); > + return -1; > + } > > ret = write_sysfs_attribute(sockfd_attr_path, sockfd_buff, > strlen(sockfd_buff)); > Best regards, -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics