[dpdk-dev] [PATCH] fixed buffer overrun in handling log messages

[dpdk-dev] [PATCH] fixed buffer overrun in handling log messages

[Han, Dongsu]

[-- Attachment #1.1: Type: text/plain, Size: 102 bytes --]

 I'm sending a proposed patch to fix the buffer overrun problem in handling
log messages.

Dongsu Han

[-- Attachment #1.2: Type: text/html, Size: 170 bytes --]

[-- Attachment #2: dpdk-patch.diff --]
[-- Type: application/octet-stream, Size: 562 bytes --]

diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index 1362109..5395078 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -204,7 +204,7 @@ rte_log_add_in_history(const char *buf, size_t size)
 
 	/* add in history */
 	memcpy(hist_buf->buf, buf, size);
-	hist_buf->buf[LOG_ELT_SIZE-1] = '\0';
+	hist_buf->buf[LOG_ELT_SIZE-1- sizeof(*hist_buf)] = '\0';
 	hist_buf->size = size;
 	STAILQ_INSERT_TAIL(&log_history, hist_buf, next);
 	rte_spinlock_unlock(&log_list_lock);

Re: [dpdk-dev] [PATCH] fixed buffer overrun in handling log messages

[Olivier MATZ]

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

Hi,

Thank you for this patch. You are right, the '\0' is written
outside the bounds of the buffer. I would suggest a minor modification
to your patch, please see attachment.

Regards,
Olivier


On 05/01/2013 08:50 PM, Han, Dongsu wrote:
>   I'm sending a proposed patch to fix the buffer overrun problem in
> handling log messages.
>
> Dongsu Han
>
>
> _______________________________________________
> dev mailing list
> dev@dpdk.org
> http://dpdk.org/ml/listinfo/dev


-- 
Olivier MATZ
Tel +33-1-39-30-92-57
www.6wind.com
================================================================================
This e-mail message, including any attachments, is for the sole use of 
the intended recipient(s) and contains information that is confidential 
and proprietary to 6WIND. All unauthorized review, use, disclosure or 
distribution is prohibited. If you are not the intended recipient, 
please contact the sender by reply e-mail and destroy all copies of the 
original message.
Ce courriel ainsi que toutes les pièces jointes, est uniquement destiné 
à son ou ses destinataires. Il contient des informations confidentielles 
qui sont la propriété de 6WIND. Toute révélation, distribution ou copie 
des informations qu'il contient est strictement interdite. Si vous avez 
reçu ce message par erreur, veuillez immédiatement le signaler à 
l'émetteur et détruire toutes les données reçues
================================================================================

[-- Attachment #2: 0001-eal-log-fix-memory-corruption.patch --]
[-- Type: text/x-patch, Size: 1702 bytes --]

>From e14f9b05dbc8aca332fa6532382df6a70ff6da25 Mon Sep 17 00:00:00 2001
From: Dongsu Han <dongsuh@cs.cmu.edu>
Date: Thu, 2 May 2013 10:14:58 +0200
Subject: eal/log: fix memory corruption

The '\0' is written outside the bounds of the log buffer, which can
result in memory corruption or display issues with log messages.

Use a new constant LOG_BUF_SIZE to store the effective size of the
buffer in struct log_history.

Acked-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Dongsu Han <dongsuh@cs.cmu.edu>
---
 lib/librte_eal/common/eal_common_log.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index 1362109..21970c5 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -64,6 +64,7 @@
 #include "eal_private.h"
 
 #define LOG_ELT_SIZE     2048
+#define LOG_BUF_SIZE     (LOG_ELT_SIZE - sizeof(struct log_history))
 
 #define LOG_HISTORY_MP_NAME "log_history"
 
@@ -196,7 +197,7 @@ rte_log_add_in_history(const char *buf, size_t size)
 	}
 
 	/* not enough room for msg, buffer go back in mempool */
-	if (size >= (LOG_ELT_SIZE - sizeof(*hist_buf))) {
+	if (size >= LOG_BUF_SIZE) {
 		rte_mempool_mp_put(log_history_mp, hist_buf);
 		rte_spinlock_unlock(&log_list_lock);
 		return -ENOBUFS;
@@ -204,7 +205,7 @@ rte_log_add_in_history(const char *buf, size_t size)
 
 	/* add in history */
 	memcpy(hist_buf->buf, buf, size);
-	hist_buf->buf[LOG_ELT_SIZE-1] = '\0';
+	hist_buf->buf[LOG_BUF_SIZE-1] = '\0';
 	hist_buf->size = size;
 	STAILQ_INSERT_TAIL(&log_history, hist_buf, next);
 	rte_spinlock_unlock(&log_list_lock);
-- 
1.7.10.4

Re: [dpdk-dev] [PATCH] fixed buffer overrun in handling log messages

[Olivier MATZ]

We can even do better, please see my next email (indeed we should not
send patch as attachment).

Olivier

On 05/02/2013 10:24 AM, Olivier MATZ wrote:
> Hi,
>
> Thank you for this patch. You are right, the '\0' is written
> outside the bounds of the buffer. I would suggest a minor modification
> to your patch, please see attachment.
>
> Regards,
> Olivier
>
>
> On 05/01/2013 08:50 PM, Han, Dongsu wrote:
>> I'm sending a proposed patch to fix the buffer overrun problem in
>> handling log messages.
>>
>> Dongsu Han
>>
>>
>> _______________________________________________
>> dev mailing list
>> dev@dpdk.org
>> http://dpdk.org/ml/listinfo/dev
>
>

[dpdk-dev] eal/log: fix memory corruption

[Olivier Matz]

From: Dongsu Han <dongsuh@cs.cmu.edu>

The '\0' is written outside the bounds of the log buffer, which can
result in memory corruption or display issues with log messages.

Use a new constant LOG_BUF_SIZE to store the effective size of the
buffer in struct log_history.

Signed-off-by: Dongsu Han <dongsuh@cs.cmu.edu>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_eal/common/eal_common_log.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index 1362109..9926ae4 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -64,6 +64,7 @@
 #include "eal_private.h"
 
 #define LOG_ELT_SIZE     2048
+#define LOG_BUF_SIZE     (LOG_ELT_SIZE - sizeof(struct log_history))
 
 #define LOG_HISTORY_MP_NAME "log_history"
 
@@ -196,7 +197,7 @@ rte_log_add_in_history(const char *buf, size_t size)
 	}
 
 	/* not enough room for msg, buffer go back in mempool */
-	if (size >= (LOG_ELT_SIZE - sizeof(*hist_buf))) {
+	if (size >= LOG_BUF_SIZE - 1) {
 		rte_mempool_mp_put(log_history_mp, hist_buf);
 		rte_spinlock_unlock(&log_list_lock);
 		return -ENOBUFS;
@@ -204,7 +205,7 @@ rte_log_add_in_history(const char *buf, size_t size)
 
 	/* add in history */
 	memcpy(hist_buf->buf, buf, size);
-	hist_buf->buf[LOG_ELT_SIZE-1] = '\0';
+	hist_buf->buf[size] = '\0';
 	hist_buf->size = size;
 	STAILQ_INSERT_TAIL(&log_history, hist_buf, next);
 	rte_spinlock_unlock(&log_list_lock);
-- 
1.7.10.4

Re: [dpdk-dev] eal/log: fix memory corruption

[Thomas Monjalon]

pushed

Thank you all
-- 
Thomas