From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751364AbdBUGuH (ORCPT ); Tue, 21 Feb 2017 01:50:07 -0500 Received: from smtp.lesbg.com ([178.62.242.175]:38244 "EHLO smtp.lesbg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751294AbdBUGsr (ORCPT ); Tue, 21 Feb 2017 01:48:47 -0500 Message-ID: <1487659710.2096.1.camel@lesbg.com> Subject: Re: [PATCH 1/2] Fix format overflows From: Jonathan Dieter To: Krzysztof Opasiak , linux-kernel@vger.kernel.org Cc: Valentina Manea , Shuah Khan , Peter Senna Tschudin , Greg Kroah-Hartman , "open list:USB OVER IP DRIVER" Date: Tue, 21 Feb 2017 08:48:30 +0200 In-Reply-To: References: <20170220205129.18194-1-jdieter@lesbg.com> Organization: Lebanon Evangelical School for Boys and Girls Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.5 (3.22.5-1.fc25) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thanks for looking at this. One quick question before I put out version two with your corrections: On Tue, 2017-02-21 at 07:12 +0100, Krzysztof Opasiak wrote: > 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); Should I also change the error messages to use real values? Ex: err("busid length %i >= %i", size, 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..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,