Skip to contentSkip to conversation

prism54: call BUG_ON() earlier

PATCH In review Dan Carpenter 2 participants 4 messages Jul 10, 2010 – Jul 12, 2010

Subsystems (2)
Kernel janitors Networking drivers (wireless)
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 prism54: call BUG_ON() earlier

This test is off by one because strlen() doesn't include the NULL terminator.

Signed-off-by Dan Carpenter <[email protected]>
drivers/net/wireless/prism54/isl_ioctl.c +1 -1
index 8d1190c..1051268 100644
@@ -2067,7 +2067,7 @@ send_simple_event(islpci_private *priv, const char *str)
 	memptr = kmalloc(IW_CUSTOM_MAX, GFP_KERNEL);
 	if (!memptr)
 		return;
-	BUG_ON(n > IW_CUSTOM_MAX);
+	BUG_ON(n >= IW_CUSTOM_MAX);
 	wrqu.data.pointer = memptr;
 	wrqu.data.length = n;
 	strcpy(memptr, str);
#1 lore raw
Quoted 11 lines · Dan Carpenter

This test is off by one because strlen() doesn't include the NULL terminator.

Signed-off-by Dan Carpenter <[email protected]>
drivers/net/wireless/prism54/isl_ioctl.c +0 -0
index 8d1190c..1051268 100644

drivers/net/wireless/prism54/isl_ioctl.c send_simple_event(islpci_private *priv, const char *str) · line 2073

Quoted 8 lines
drivers/net/wireless/prism54/isl_ioctl.c +1 -1
@@ -2067,7 +2067,7 @@ send_simple_event(islpci_private *priv, const char *str)
        memptr = kmalloc(IW_CUSTOM_MAX, GFP_KERNEL);
        if (!memptr)
                return;
-       BUG_ON(n > IW_CUSTOM_MAX);
+       BUG_ON(n >= IW_CUSTOM_MAX);
        wrqu.data.pointer = memptr;
        wrqu.data.length = n;
        strcpy(memptr, str);

send_simple_event() never passes a NULL terminated string though. What does this fix today? If nothing then better leave as-is.

Luis

author
#2 lore raw
Quoted 24 lines · Luis R. Rodriguez
Quoted 11 lines · Dan Carpenter

This test is off by one because strlen() doesn't include the NULL terminator.

Signed-off-by Dan Carpenter <[email protected]>
drivers/net/wireless/prism54/isl_ioctl.c +0 -0
index 8d1190c..1051268 100644

drivers/net/wireless/prism54/isl_ioctl.c send_simple_event(islpci_private *priv, const char *str) · line 2073

Quoted 8 lines
drivers/net/wireless/prism54/isl_ioctl.c +1 -1
@@ -2067,7 +2067,7 @@ send_simple_event(islpci_private *priv, const char *str)
        memptr = kmalloc(IW_CUSTOM_MAX, GFP_KERNEL);
        if (!memptr)
                return;
-       BUG_ON(n > IW_CUSTOM_MAX);
+       BUG_ON(n >= IW_CUSTOM_MAX);
        wrqu.data.pointer = memptr;
        wrqu.data.length = n;
        strcpy(memptr, str);

send_simple_event() never passes a NULL terminated string though. What does this fix today? If nothing then better leave as-is.

Quoted 1 line

Luis

It doesn't fix any bugs in the current code, but it's a necessary clean up.

        memptr = kmalloc(IW_CUSTOM_MAX, GFP_KERNEL);
	                 ^^^^^^^^^^^^^
	This is the size of memptr.

        if (!memptr)
                return;
        BUG_ON(n > IW_CUSTOM_MAX);

	^^^^^^^^^^^^^^^^^^^^^^^^^^
	This is an off-by-one check.

        wrqu.data.pointer = memptr;
        wrqu.data.length = n;
        strcpy(memptr, str);
	^^^^^^^^^^^^^^^^^^^^

	This would be a silent memory corruption.

In the current code we only use short event strings so the check isn't needed. But we should either correct the check or remove it.

regards,
dan carpenter

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

author
#3 lore raw

Of course the main reasons is that it silences a static checkers warning. :P

drivers/net/wireless/prism54/isl_ioctl.c +2073 send_simple_event(12)

	error: strcpy() 'str' too large for 'memptr' (257 vs 256)

regards,
dan carpenter