From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754499AbdCPPHI (ORCPT ); Thu, 16 Mar 2017 11:07:08 -0400 Received: from resqmta-ch2-08v.sys.comcast.net ([69.252.207.40]:34834 "EHLO resqmta-ch2-08v.sys.comcast.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753609AbdCPPHE (ORCPT ); Thu, 16 Mar 2017 11:07:04 -0400 Reply-To: shuah@kernel.org Subject: Re: [PATCH v4 1/2] usbip: Fix-format-overflow References: <20170222181803.16373-1-jdieter@lesbg.com> <20170227083108.16391-1-jdieter@lesbg.com> To: Jonathan Dieter , linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Krzysztof Opasiak Cc: Valentina Manea , Peter Senna Tschudin , "open list:USB OVER IP DRIVER" From: Shuah Khan Message-ID: <0dd8ccff-37c7-c7fd-0e33-84e6f69ff4d7@kernel.org> Date: Thu, 16 Mar 2017 09:04:54 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20170227083108.16391-1-jdieter@lesbg.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-CMAE-Envelope: MS4wfKtmfx6u72hGmrGAL/VNSlFe7EDn+L3/+/nxO1+Z1Ph6Jtoe/fFp2hIb/qvkMTk/h6ceThgL1Rmr+eboyxNf7Dv7RLEuibeZ4++IfvKalwsGPzLTCJ+3 wplZk9/LV8aQ+vf9kd/XBfR2DVDqKczbSU7BfhsH5BeyjOelqqV9zvoW8vvyU+KpabkNCcjmRPA/A93ZrAyn/XeebVg80/tXWbikhttl+UXtFeUFp4kWArVp cS+RXPro9p/bK+XM3a/rHgPu9DwSpspG6E67wQrTt4FNjDmlmeIO4TgLakhekw025DQRp9kKZ7XjAOvd1j3eDIwNBPRBENc0jPsokSZETkB/+8g8bUWgviaQ T2bTre926ExxFsZ2GLKIN5HUkH0qewF2crz6tBFsmNJRiZeLk5Q= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/27/2017 01:31 AM, Jonathan Dieter wrote: > 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 Greg, Please pick this up. Acked-by: Shuah Khan thanks, -- Shuah > --- > Changes since v3 > * Cast sizeof to long unsigned when printing errors to fix building for 32-bit > architectures > > tools/usb/usbip/libsrc/usbip_common.c | 9 ++++++++- > tools/usb/usbip/libsrc/usbip_host_common.c | 28 +++++++++++++++++++++++----- > 2 files changed, 31 insertions(+), 6 deletions(-) > > diff --git a/tools/usb/usbip/libsrc/usbip_common.c b/tools/usb/usbip/libsrc/usbip_common.c > index ac73710..1517a23 100644 > --- a/tools/usb/usbip/libsrc/usbip_common.c > +++ b/tools/usb/usbip/libsrc/usbip_common.c > @@ -215,9 +215,16 @@ 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, sizeof(busid), "%s:%d.%d", > + udev->busid, udev->bConfigurationValue, i); > + if (size < 0 || (unsigned int)size >= sizeof(busid)) { > + err("busid length %i >= %lu or < 0", size, > + (long unsigned)sizeof(busid)); > + 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..6ff7b60 100644 > --- a/tools/usb/usbip/libsrc/usbip_host_common.c > +++ b/tools/usb/usbip/libsrc/usbip_host_common.c > @@ -40,13 +40,20 @@ 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, sizeof(status_attr_path), > + "%s/usbip_status", udev->path); > + if (size < 0 || (unsigned int)size >= sizeof(status_attr_path)) { > + err("usbip_status path length %i >= %lu or < 0", size, > + (long unsigned)sizeof(status_attr_path)); > + return -1; > + } > + > > fd = open(status_attr_path, O_RDONLY); > if (fd < 0) { > @@ -218,6 +225,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 +245,20 @@ 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 < 0 || (unsigned int)size >= sizeof(sockfd_attr_path)) { > + err("exported device path length %i >= %lu or < 0", size, > + (long unsigned)sizeof(sockfd_attr_path)); > + return -1; > + } > > - snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", sockfd); > + size = snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", sockfd); > + if (size < 0 || (unsigned int)size >= sizeof(sockfd_buff)) { > + err("socket length %i >= %lu or < 0", size, > + (long unsigned)sizeof(sockfd_buff)); > + return -1; > + } > > ret = write_sysfs_attribute(sockfd_attr_path, sockfd_buff, > strlen(sockfd_buff)); >