Skip to contentSkip to conversation

dma-debug: off by one issue

PATCH In review Dan Carpenter 3 participants 7 messages Apr 2, 2010 – Apr 7, 2010

Subsystems (2)
Kernel janitors Library code
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 dma-debug: off by one issue

We need to reserve the last character for the NULL terminator.

Signed-off-by Dan Carpenter <[email protected]>
lib/dma-debug.c +1 -1
index ba8b670..01e6427 100644
@@ -570,7 +570,7 @@ static ssize_t filter_write(struct file *file, const char __user *userbuf,
 	 * Now parse out the first token and use it as the name for the
 	 * driver to filter for.
 	 */
-	for (i = 0; i < NAME_MAX_LEN; ++i) {
+	for (i = 0; i < NAME_MAX_LEN - 1; ++i) {
 		current_driver_name[i] = buf[i];
 		if (isspace(buf[i]) || buf[i] = ' ' || buf[i] = 0)
 			break;
#1 lore raw
Quoted 1 line · Dan Carpenter

We need to reserve the last character for the NULL terminator.

The character is already reserved by the user-to-kernel copy earlier in the function.

Joerg

Quoted 16 lines
Signed-off-by Dan Carpenter <[email protected]>
lib/dma-debug.c +1 -1
index ba8b670..01e6427 100644
@@ -570,7 +570,7 @@ static ssize_t filter_write(struct file *file, const char __user *userbuf,
 	 * Now parse out the first token and use it as the name for the
 	 * driver to filter for.
 	 */
-	for (i = 0; i < NAME_MAX_LEN; ++i) {
+	for (i = 0; i < NAME_MAX_LEN - 1; ++i) {
 		current_driver_name[i] = buf[i];
 		if (isspace(buf[i]) || buf[i] = ' ' || buf[i] = 0)
 			break;
author
#2 lore raw
Quoted 8 lines · Joerg Roedel
Quoted 1 line · Dan Carpenter

We need to reserve the last character for the NULL terminator.

The character is already reserved by the user-to-kernel copy earlier in the function.

Joerg

Yes, but the original code requires more code reading to audit. Let's change the commit message to say that it's a clean up patch.

regards,
dan carpenter

Quoted 21 lines
Quoted 8 lines
Signed-off-by Dan Carpenter <[email protected]>
lib/dma-debug.c +0 -0
index ba8b670..01e6427 100644

lib/dma-debug.c static ssize_t filter_write(struct file *file, const char __user *userbuf, · line 576

Quoted 8 lines
lib/dma-debug.c +1 -1
@@ -570,7 +570,7 @@ static ssize_t filter_write(struct file *file, const char __user *userbuf,
 	 * Now parse out the first token and use it as the name for the
 	 * driver to filter for.
 	 */
-	for (i = 0; i < NAME_MAX_LEN; ++i) {
+	for (i = 0; i < NAME_MAX_LEN - 1; ++i) {
 		current_driver_name[i] = buf[i];
 		if (isspace(buf[i]) || buf[i] = ' ' || buf[i] = 0)
 			break;

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

#3 lore raw
Quoted 7 lines · Dan Carpenter
Quoted 2 lines · Joerg Roedel

The character is already reserved by the user-to-kernel copy earlier in the function.

Yes, but the original code requires more code reading to audit. Let's change the commit message to say that it's a clean up patch.

In this case its better to remove the

	current_driver_name[i] = 0;

line just after the loop.

Joerg

#4 lore raw
Quoted 6 lines · Joerg Roedel
Quoted 5 lines · Dan Carpenter
Quoted 2 lines · Joerg Roedel

The character is already reserved by the user-to-kernel copy earlier in the function.

Quoted 8 lines
Quoted 2 lines

Yes, but the original code requires more code reading to audit. Let's change the commit message to say that it's a clean up patch.

In this case its better to remove the

	current_driver_name[i] = 0;

line just after the loop.

Ah no, thats nonsense. Mind to resend the patch with a new commit-message? I'll queue it then tomorrow.

Joerg

author
#5 lore raw

PATCH v2 1/1 dma-debug: cleanup for loop

Earlier in this function we set the last byte of "buf" to NULL so we always hit the break statement and "i" is never equal to NAME_MAX_LEN. This patch doesn't change how the driver works but it silences a Smatch warning and it makes it clearer that we don't write past the end of the array.

Signed-off-by Dan Carpenter <[email protected]>
lib/dma-debug.c +1 -1
index ba8b670..01e6427 100644
@@ -570,7 +570,7 @@ static ssize_t filter_write(struct file *file, const char __user *userbuf,
 	 * Now parse out the first token and use it as the name for the
 	 * driver to filter for.
 	 */
-	for (i = 0; i < NAME_MAX_LEN; ++i) {
+	for (i = 0; i < NAME_MAX_LEN - 1; ++i) {
 		current_driver_name[i] = buf[i];
 		if (isspace(buf[i]) || buf[i] = ' ' || buf[i] = 0)
 			break;
Applied
#6 lore raw

Re: [patch v2] dma-debug: cleanup for loop

Quoted 13 lines · Dan Carpenter

Earlier in this function we set the last byte of "buf" to NULL so we always hit the break statement and "i" is never equal to NAME_MAX_LEN. This patch doesn't change how the driver works but it silences a Smatch warning and it makes it clearer that we don't write past the end of the array.

Signed-off-by Dan Carpenter <[email protected]>
lib/dma-debug.c +0 -0
index ba8b670..01e6427 100644

lib/dma-debug.c static ssize_t filter_write(struct file *file, const char __user *userbuf, · line 576

Quoted 8 lines
lib/dma-debug.c +1 -1
@@ -570,7 +570,7 @@ static ssize_t filter_write(struct file *file, const char __user *userbuf,
 	 * Now parse out the first token and use it as the name for the
 	 * driver to filter for.
 	 */
-	for (i = 0; i < NAME_MAX_LEN; ++i) {
+	for (i = 0; i < NAME_MAX_LEN - 1; ++i) {
 		current_driver_name[i] = buf[i];
 		if (isspace(buf[i]) || buf[i] = ' ' || buf[i] = 0)
 			break;

Applied, thanks.