From f15f084ff11519172c67fac4dde61daf4e9ad345 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Tue, 17 Jul 2018 14:32:24 -0700 Subject: [PATCH] pktgen: convert safe uses of strncpy() to strcpy() to avoid string truncation warning MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit GCC 8 complains: net/core/pktgen.c: In function ‘pktgen_if_write’: net/core/pktgen.c:1419:4: warning: ‘strncpy’ output may be truncated copying between 0 and 31 bytes from a string of length 127 [-Wstringop-truncation] strncpy(pkt_dev->src_max, buf, len); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ net/core/pktgen.c:1399:4: warning: ‘strncpy’ output may be truncated copying between 0 and 31 bytes from a string of length 127 [-Wstringop-truncation] strncpy(pkt_dev->src_min, buf, len); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ net/core/pktgen.c:1290:4: warning: ‘strncpy’ output may be truncated copying between 0 and 31 bytes from a string of length 127 [-Wstringop-truncation] strncpy(pkt_dev->dst_max, buf, len); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ net/core/pktgen.c:1268:4: warning: ‘strncpy’ output may be truncated copying between 0 and 31 bytes from a string of length 127 [-Wstringop-truncation] strncpy(pkt_dev->dst_min, buf, len); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ There is no bug here, but the code is not perfect either. It copies sizeof(pkt_dev->/member/) - 1 from user space into buf, and then does a strcmp(pkt_dev->/member/, buf) hence assuming buf will be null-terminated and shorter than pkt_dev->/member/ (pkt_dev->/member/ is never explicitly null-terminated, and strncpy() doesn't have to null-terminate so the assumption must be on buf). The use of strncpy() without explicit null-termination looks suspicious. Convert to use straight strcpy(). strncpy() would also null-pad the output, but that's clearly unnecessary since the author calls memset(pkt_dev->/member/, 0, sizeof(..)); prior to strncpy(), anyway. While at it format the code for "dst_min", "dst_max", "src_min" and "src_max" in the same way by removing extra new lines in one case. Signed-off-by: Jakub Kicinski Reviewed-by: Jiong Wang Signed-off-by: David S. Miller --- net/core/pktgen.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 49368e21d228..308ed04984de 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -1265,7 +1265,7 @@ static ssize_t pktgen_if_write(struct file *file, buf[len] = 0; if (strcmp(buf, pkt_dev->dst_min) != 0) { memset(pkt_dev->dst_min, 0, sizeof(pkt_dev->dst_min)); - strncpy(pkt_dev->dst_min, buf, len); + strcpy(pkt_dev->dst_min, buf); pkt_dev->daddr_min = in_aton(pkt_dev->dst_min); pkt_dev->cur_daddr = pkt_dev->daddr_min; } @@ -1280,14 +1280,12 @@ static ssize_t pktgen_if_write(struct file *file, if (len < 0) return len; - if (copy_from_user(buf, &user_buffer[i], len)) return -EFAULT; - buf[len] = 0; if (strcmp(buf, pkt_dev->dst_max) != 0) { memset(pkt_dev->dst_max, 0, sizeof(pkt_dev->dst_max)); - strncpy(pkt_dev->dst_max, buf, len); + strcpy(pkt_dev->dst_max, buf); pkt_dev->daddr_max = in_aton(pkt_dev->dst_max); pkt_dev->cur_daddr = pkt_dev->daddr_max; } @@ -1396,7 +1394,7 @@ static ssize_t pktgen_if_write(struct file *file, buf[len] = 0; if (strcmp(buf, pkt_dev->src_min) != 0) { memset(pkt_dev->src_min, 0, sizeof(pkt_dev->src_min)); - strncpy(pkt_dev->src_min, buf, len); + strcpy(pkt_dev->src_min, buf); pkt_dev->saddr_min = in_aton(pkt_dev->src_min); pkt_dev->cur_saddr = pkt_dev->saddr_min; } @@ -1416,7 +1414,7 @@ static ssize_t pktgen_if_write(struct file *file, buf[len] = 0; if (strcmp(buf, pkt_dev->src_max) != 0) { memset(pkt_dev->src_max, 0, sizeof(pkt_dev->src_max)); - strncpy(pkt_dev->src_max, buf, len); + strcpy(pkt_dev->src_max, buf); pkt_dev->saddr_max = in_aton(pkt_dev->src_max); pkt_dev->cur_saddr = pkt_dev->saddr_max; } -- 2.11.0