Re: [PATCH 1/2] Fix format overflows

[Jonathan Dieter]

On Tue, 2017-02-21 at 08:48 +0200, Jonathan Dieter wrote:
> On Tue, 2017-02-21 at 07:12 +0100, Krzysztof Opasiak wrote:
> > Hi,
> >  
> > W dniu 2017-02-20 o 21:51, Jonathan Dieter pisze: 
> > > + 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));

On further reflection, I do think changing the error messages to use
sizeof() is cleaner.

If you disagree, please let me know.  I don't have a strong feeling
about it one way or the other.

The next revision uses sizeof() in the places you indicated and changes
size to an unsigned int since we're now comparing it to the result of
sizeof().

Thanks again,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Fix format overflows

[Jonathan Dieter]

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,
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Fix format overflows

[Krzysztof Opasiak]

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 Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html