* [PATCH] staging: ced1401: userspace: use1401.c: Cleaning up potential strncpy missing null-terminate
@ 2014-06-14 22:41 Rickard Strandqvist
2014-06-14 22:41 ` Rickard Strandqvist
0 siblings, 1 reply; 6+ messages in thread
From: Rickard Strandqvist @ 2014-06-14 22:41 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rickard Strandqvist
Cc: Monam Agarwal, devel, linux-kernel
Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
There is also a wrong use of strncat, it should have used sizeof - strlen.
This was found using a static code analysis program called cppcheck.
Rickard Strandqvist (1):
staging: ced1401: userspace: use1401.c: Cleaning up potential strncpy missing null-terminate
drivers/staging/ced1401/userspace/use1401.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
--
1.7.10.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] staging: ced1401: userspace: use1401.c: Cleaning up potential strncpy missing null-terminate
2014-06-14 22:41 [PATCH] staging: ced1401: userspace: use1401.c: Cleaning up potential strncpy missing null-terminate Rickard Strandqvist
@ 2014-06-14 22:41 ` Rickard Strandqvist
2014-06-16 19:01 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Rickard Strandqvist @ 2014-06-14 22:41 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rickard Strandqvist
Cc: Monam Agarwal, devel, linux-kernel
Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
drivers/staging/ced1401/userspace/use1401.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/ced1401/userspace/use1401.c b/drivers/staging/ced1401/userspace/use1401.c
index 7b8a222..98d0301 100644
--- a/drivers/staging/ced1401/userspace/use1401.c
+++ b/drivers/staging/ced1401/userspace/use1401.c
@@ -693,7 +693,7 @@ U14API(short) U14DriverName(short hand, char* pBuf, unsigned short wMax)
case 3: pName = "HSS"; break;
default: pName = "???"; break;
}
- strncpy(pBuf, pName, wMax); // Copy the correct name to return
+ strlcpy(pBuf, pName, wMax); /* Copy the correct name to return */
return U14ERR_NOERROR;
}
@@ -1079,7 +1079,7 @@ U14API(short) U14NameOf1401(short hand, char* pBuf, unsigned short wMax)
case U14TYPEPOWER3:pName = "Power1401-3"; break;
default: pName = "Unknown";
}
- strncpy(pBuf, pName, wMax);
+ strlcpy(pBuf, pName, wMax);
}
return sErr;
}
@@ -2891,10 +2891,8 @@ U14API(unsigned int) U14Ld(short hand, const char* vl, const char* str)
++dwIndex; // Keep count of command number, first is 1
szFName[iLoop2]=(char)0; // null terminate name of command
- strncpy(szLastName, szFName, sizeof(szLastName)); // Save for error info
- szLastName[sizeof(szLastName)-1] = 0;
- strncat(szLastName, szFExt, sizeof(szLastName)); // with extension included
- szLastName[sizeof(szLastName)-1] = 0;
+ strlcpy(szLastName, szFName, sizeof(szLastName)); /* Save for error info */
+ strlcat(szLastName, szFExt, sizeof(szLastName)); /* with extension included */
U14SendString(hand, szFName); // ask if loaded
U14SendString(hand, ";ERR;"); // add err return
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: ced1401: userspace: use1401.c: Cleaning up potential strncpy missing null-terminate
2014-06-14 22:41 ` Rickard Strandqvist
@ 2014-06-16 19:01 ` Dan Carpenter
2014-06-16 19:09 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2014-06-16 19:01 UTC (permalink / raw)
To: Rickard Strandqvist
Cc: Greg Kroah-Hartman, devel, linux-kernel, Monam Agarwal
On Sun, Jun 15, 2014 at 12:41:47AM +0200, Rickard Strandqvist wrote:
> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
Generally in the kernel we allow strncpy() if people want to use it.
Let's not start doing sed replacements of these if it doesn't cause a
problem.
Also strncpy() always writes n number of bytes. If the source string is
not long enough then NUL characters are written. This is sometimes
important to prevent information leaks so switching to strcpy() can
cause security bugs.
>
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
> drivers/staging/ced1401/userspace/use1401.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/ced1401/userspace/use1401.c b/drivers/staging/ced1401/userspace/use1401.c
> index 7b8a222..98d0301 100644
> --- a/drivers/staging/ced1401/userspace/use1401.c
> +++ b/drivers/staging/ced1401/userspace/use1401.c
> @@ -693,7 +693,7 @@ U14API(short) U14DriverName(short hand, char* pBuf, unsigned short wMax)
> case 3: pName = "HSS"; break;
> default: pName = "???"; break;
> }
> - strncpy(pBuf, pName, wMax); // Copy the correct name to return
> + strlcpy(pBuf, pName, wMax); /* Copy the correct name to return */
>
> return U14ERR_NOERROR;
> }
No one calls U14DriverName() so just delete this function.
> @@ -1079,7 +1079,7 @@ U14API(short) U14NameOf1401(short hand, char* pBuf, unsigned short wMax)
> case U14TYPEPOWER3:pName = "Power1401-3"; break;
> default: pName = "Unknown";
> }
> - strncpy(pBuf, pName, wMax);
> + strlcpy(pBuf, pName, wMax);
> }
> return sErr;
> }
Same thing. Just delete the whole function.
> @@ -2891,10 +2891,8 @@ U14API(unsigned int) U14Ld(short hand, const char* vl, const char* str)
> ++dwIndex; // Keep count of command number, first is 1
> szFName[iLoop2]=(char)0; // null terminate name of command
>
> - strncpy(szLastName, szFName, sizeof(szLastName)); // Save for error info
> - szLastName[sizeof(szLastName)-1] = 0;
> - strncat(szLastName, szFExt, sizeof(szLastName)); // with extension included
> - szLastName[sizeof(szLastName)-1] = 0;
> + strlcpy(szLastName, szFName, sizeof(szLastName)); /* Save for error info */
> + strlcat(szLastName, szFExt, sizeof(szLastName)); /* with extension included */
>
> U14SendString(hand, szFName); // ask if loaded
> U14SendString(hand, ";ERR;"); // add err return
Delete again.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: ced1401: userspace: use1401.c: Cleaning up potential strncpy missing null-terminate
2014-06-16 19:01 ` Dan Carpenter
@ 2014-06-16 19:09 ` Dan Carpenter
2014-06-16 22:14 ` Rickard Strandqvist
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2014-06-16 19:09 UTC (permalink / raw)
To: Rickard Strandqvist
Cc: devel, Greg Kroah-Hartman, linux-kernel, Monam Agarwal
On Mon, Jun 16, 2014 at 10:01:14PM +0300, Dan Carpenter wrote:
>
> No one calls U14DriverName() so just delete this function.
Oh. This is a userspace library or something? I'm not sure what to do.
How did you compile test this? Does your platform even provide
strlcpy()?
Anyway, don't delete it like I said, but I still don't think your patch
helps here.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: ced1401: userspace: use1401.c: Cleaning up potential strncpy missing null-terminate
2014-06-16 19:09 ` Dan Carpenter
@ 2014-06-16 22:14 ` Rickard Strandqvist
2014-06-17 5:56 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Rickard Strandqvist @ 2014-06-16 22:14 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, Greg Kroah-Hartman, linux-kernel@vger.kernel.org,
Monam Agarwal
2014-06-16 21:09 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Mon, Jun 16, 2014 at 10:01:14PM +0300, Dan Carpenter wrote:
>>
>> No one calls U14DriverName() so just delete this function.
>
> Oh. This is a userspace library or something? I'm not sure what to do.
>
> How did you compile test this? Does your platform even provide
> strlcpy()?
>
> Anyway, don't delete it like I said, but I still don't think your patch
> helps here.
>
> regards,
> dan carpenter
>
Hi
I saw that these functions are not used, had planned type it in the
cover letter to :-(
Thought of userspec, but then it should definitely ensure the NULL char?
Kind regards
Rickard Strandqvist
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: ced1401: userspace: use1401.c: Cleaning up potential strncpy missing null-terminate
2014-06-16 22:14 ` Rickard Strandqvist
@ 2014-06-17 5:56 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2014-06-17 5:56 UTC (permalink / raw)
To: Rickard Strandqvist
Cc: devel, Greg Kroah-Hartman, linux-kernel@vger.kernel.org,
Monam Agarwal
On Tue, Jun 17, 2014 at 12:14:18AM +0200, Rickard Strandqvist wrote:
> 2014-06-16 21:09 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> > On Mon, Jun 16, 2014 at 10:01:14PM +0300, Dan Carpenter wrote:
> >>
> >> No one calls U14DriverName() so just delete this function.
> >
> > Oh. This is a userspace library or something? I'm not sure what to do.
> >
> > How did you compile test this? Does your platform even provide
> > strlcpy()?
> >
> > Anyway, don't delete it like I said, but I still don't think your patch
> > helps here.
> >
> > regards,
> > dan carpenter
> >
>
> Hi
>
> I saw that these functions are not used, had planned type it in the
> cover letter to :-(
>
> Thought of userspec, but then it should definitely ensure the NULL char?
The strings are always 4 characters long. If the user start passing 3
character buffers then it will *never* work. That means the bug will be
caught on the first run in testing.
I don't worry about those kinds of bugs very much. It's only a concern
if it works 99 times and fails 1 time.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-17 5:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-14 22:41 [PATCH] staging: ced1401: userspace: use1401.c: Cleaning up potential strncpy missing null-terminate Rickard Strandqvist
2014-06-14 22:41 ` Rickard Strandqvist
2014-06-16 19:01 ` Dan Carpenter
2014-06-16 19:09 ` Dan Carpenter
2014-06-16 22:14 ` Rickard Strandqvist
2014-06-17 5:56 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox