Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1585573 > unrolled thread

[PATCH v2 1/2] usbip: Fix-format-overflow

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 1/2] usbip: Fix-format-overflow Jonathan Dieter <jdieter@lesbg.com> - 2017-02-21 19:00 +0100
    Re: [PATCH v2 1/2] usbip: Fix-format-overflow Krzysztof Opasiak <k.opasiak@samsung.com> - 2017-02-22 07:00 +0100
      [PATCH v3 1/2] usbip: Fix-format-overflow Jonathan Dieter <jdieter@lesbg.com> - 2017-02-22 19:20 +0100
        [PATCH v3 2/2] usbip: Fix implicit fallthrough warning Jonathan Dieter <jdieter@lesbg.com> - 2017-02-22 19:20 +0100
        [PATCH v4 1/2] usbip: Fix-format-overflow Jonathan Dieter <jdieter@lesbg.com> - 2017-02-27 10:00 +0100
          [PATCH v4 2/2] usbip: Fix implicit fallthrough warning Jonathan Dieter <jdieter@lesbg.com> - 2017-02-27 10:00 +0100
      Re: [PATCH v2 1/2] usbip: Fix-format-overflow Jonathan Dieter <jdieter@lesbg.com> - 2017-02-22 19:20 +0100

#1585573 — [PATCH v2 1/2] usbip: Fix-format-overflow

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..01dd4b2 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];
+	unsigned 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 >= sizeof(busid)) {
+		err("busid length %u >= %lu", 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..398c1fa 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];
+	unsigned 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 >= sizeof(status_attr_path)) {
+		err("usbip_status path length %u >= %lu", size, sizeof(status_attr_path));
+		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];
+	unsigned 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 >= sizeof(sockfd_attr_path)) {
+		err("exported device path length %u >= %lu", size, 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 >= sizeof(sockfd_buff)) {
+		err("socket length %u >= %lu", size, sizeof(sockfd_buff));
+		return -1;
+	}
 
 	ret = write_sysfs_attribute(sockfd_attr_path, sockfd_buff,
 				    strlen(sockfd_buff));
-- 
2.9.3

[toc] | [next] | [standalone]

#1585940

Hi,

W dniu 2017-02-21 o 18:57, 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..01dd4b2 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];
> +	unsigned int size;

I'm not really convinced to use unsigned here. snprintf() is declared to 
return signed integer so we should assume that some of its 
implementation may return negative error code. Any rationale to this 
instead of just doing a cast for comparsion but signed value to print error?

Best regards
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics

[toc] | [prev] | [next] | [standalone]

#1586367 — [PATCH v3 1/2] usbip: Fix-format-overflow

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>
---
Changes since v2
 * Converted size back to int because snprintf may return < 0 if there's an
   error
 * Expanded error messages to indicate failure may be because size < 0

 tools/usb/usbip/libsrc/usbip_common.c      |  8 +++++++-
 tools/usb/usbip/libsrc/usbip_host_common.c | 28 +++++++++++++++++++++++-----
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/tools/usb/usbip/libsrc/usbip_common.c b/tools/usb/usbip/libsrc/usbip_common.c
index ac73710..66017d7 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, 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, 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..6fb91d9 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,
+		    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,
+		    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,
+		    sizeof(sockfd_buff));
+		return -1;
+	}
 
 	ret = write_sysfs_attribute(sockfd_attr_path, sockfd_buff,
 				    strlen(sockfd_buff));
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1586371 — [PATCH v3 2/2] usbip: Fix implicit fallthrough warning

GCC 7 now warns when switch statements fall through implicitly, and with
-Werror enabled in configure.ac, that makes these tools unbuildable.

We fix this by notifying the compiler that this particular case statement
is meant to fall through.

Reviewed-by: Peter Senna Tschudin <peter.senna@gmail.com>
Signed-off-by: Jonathan Dieter <jdieter@lesbg.com>
---
 tools/usb/usbip/src/usbip.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/usb/usbip/src/usbip.c b/tools/usb/usbip/src/usbip.c
index d7599d9..73d8eee 100644
--- a/tools/usb/usbip/src/usbip.c
+++ b/tools/usb/usbip/src/usbip.c
@@ -176,6 +176,8 @@ int main(int argc, char *argv[])
 			break;
 		case '?':
 			printf("usbip: invalid option\n");
+			/* Terminate after printing error */
+			/* FALLTHRU */
 		default:
 			usbip_usage();
 			goto out;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1588551 — [PATCH v4 1/2] usbip: Fix-format-overflow

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>
---
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));
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1588552 — [PATCH v4 2/2] usbip: Fix implicit fallthrough warning

GCC 7 now warns when switch statements fall through implicitly, and with
-Werror enabled in configure.ac, that makes these tools unbuildable.

We fix this by notifying the compiler that this particular case statement
is meant to fall through.

Reviewed-by: Peter Senna Tschudin <peter.senna@gmail.com>
Signed-off-by: Jonathan Dieter <jdieter@lesbg.com>
---
 tools/usb/usbip/src/usbip.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/usb/usbip/src/usbip.c b/tools/usb/usbip/src/usbip.c
index d7599d9..73d8eee 100644
--- a/tools/usb/usbip/src/usbip.c
+++ b/tools/usb/usbip/src/usbip.c
@@ -176,6 +176,8 @@ int main(int argc, char *argv[])
 			break;
 		case '?':
 			printf("usbip: invalid option\n");
+			/* Terminate after printing error */
+			/* FALLTHRU */
 		default:
 			usbip_usage();
 			goto out;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1586369

On Wed, 2017-02-22 at 06:49 +0100, Krzysztof Opasiak wrote:
> Hi,
> 
> W dniu 2017-02-21 o 18:57, Jonathan Dieter pisze:
<snip>
> >  	char busid[SYSFS_BUS_ID_SIZE];
> > +	unsigned int size;
> 
> I'm not really convinced to use unsigned here. snprintf() is declared
> to 
> return signed integer so we should assume that some of its 
> implementation may return negative error code. Any rationale to this 
> instead of just doing a cast for comparsion but signed value to print
> error?

No, you're absolutely right.  I've fixed this and am sending the next
revision now.

Jonathan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web