linux-kernel - Re: [PATCH] arch: tile: gxio: mpipe.c: Cleaning up missing null-terminate in conjunction with strncpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 5 Aug 2014 16:24:24 -0400
From:	Chris Metcalf <cmetcalf@...era.com>
To:	Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>,
	"David S. Miller" <davem@...emloft.net>
CC:	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] arch: tile: gxio: mpipe.c:  Cleaning up missing null-terminate
 in conjunction with strncpy

On 7/26/2014 10:03 AM, Rickard Strandqvist wrote:
> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
>
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
> ---

While using strlcpy() instead of strncpy() and an explicit NUL termination
is obviously an improvement, on reflection it doesn't feel like the best
solution here - and likely in many similar APIs.  A partial string argument
should not be passed on to other components; it's just an error by
definition and should be treated that way.  While we could check the
strlcpy() return value explicitly against the length, this is somewhat
awkward as it requires naming the length twice, and also means we end
up doing the copy pointlessly if we're about to return an error anyway.

The following change includes Yet Another String Copy function; this may
or may not be a fabulous idea, but at least it encapsulates the error
checking in a way that seems like it makes the call sites cleaner.


From: Chris Metcalf <cmetcalf@...era.com>
Date: Tue, 5 Aug 2014 16:16:13 -0400
Subject: [PATCH] tile: avoid errors from truncating long strings in mpipe gxio

Using strncpy() will just silently truncate long strings; we should
instead return an appropriate error.  Using strlcpy() would suffer from
the same problem.  Instead, use strlen()+memcpy(), and add an
error-checking step to make sure the lengths are reasonable.

I called the convenience wrapper strscpy(), and a case could be made for
making it more generic (possibly with a better name), but that seems
outside the scope of this initial commit.

Signed-off-by: Chris Metcalf <cmetcalf@...era.com>
---
  arch/tile/gxio/mpipe.c | 30 +++++++++++++++++++++++++-----
  1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c
index 5301a9ffbae1..8b3c201dfdb7 100644
--- a/arch/tile/gxio/mpipe.c
+++ b/arch/tile/gxio/mpipe.c
@@ -29,6 +29,25 @@
  /* HACK: Avoid pointless "shadow" warnings. */
  #define link link_shadow

+/*
+ * Use this routine to avoid copying too-long strings.  Unlike strncpy
+ * or strlcpy, we don't enable programmers who don't check return codes;
+ * partially-copied strings can be problematic.  The routine returns
+ * the total number of bytes copied (including the trailing NUL) or
+ * zero if the buffer wasn't big enough.
+ */
+static size_t strscpy(char *dest, const char *src, size_t size)
+{
+       size_t ret = strlen(src) + 1;
+       if (ret > size) {
+               if (size)
+                       dest[0] = '\0';
+               return 0;
+       }
+       memcpy(dest, src, ret);
+       return ret;
+}
+
  int gxio_mpipe_init(gxio_mpipe_context_t *context, unsigned int mpipe_index)
  {
         char file[32];
@@ -511,8 +530,8 @@ int gxio_mpipe_link_instance(const char *link_name)
         if (!context)
                 return GXIO_ERR_NO_DEVICE;

-       strncpy(name.name, link_name, sizeof(name.name));
-       name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
+       if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
+               return GXIO_ERR_NO_DEVICE;

         return gxio_mpipe_info_instance_aux(context, name);
  }
@@ -529,7 +548,8 @@ int gxio_mpipe_link_enumerate_mac(int idx, char *link_name, uint8_t *link_mac)

         rv = gxio_mpipe_info_enumerate_aux(context, idx, &name, &mac);
         if (rv >= 0) {
-               strncpy(link_name, name.name, sizeof(name.name));
+               if (strscpy(link_name, name.name, sizeof(name.name)) == 0)
+                       return GXIO_ERR_INVAL_MEMORY_SIZE;
                 memcpy(link_mac, mac.mac, sizeof(mac.mac));
         }

@@ -545,8 +565,8 @@ int gxio_mpipe_link_open(gxio_mpipe_link_t *link,
         _gxio_mpipe_link_name_t name;
         int rv;

-       strncpy(name.name, link_name, sizeof(name.name));
-       name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0';
+       if (strscpy(name.name, link_name, sizeof(name.name)) == 0)
+               return GXIO_ERR_NO_DEVICE;

         rv = gxio_mpipe_link_open_aux(context, name, flags);
         if (rv < 0)
--
1.8.3.1

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/