Messages in this thread | | | Subject | Re: [PATCH 1/2] Fix format overflows | From | Krzysztof Opasiak <> | Date | Tue, 21 Feb 2017 07:12:27 +0100 |
| |
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 <peter.senna@gmail.com> > Signed-off-by: Jonathan Dieter <jdieter@lesbg.com> > --- > 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
| |