linux-kernel - Re: [PATCH] staging: ced1401: userspace: use1401.c: Cleaning up potential strncpy missing null-terminate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140616190114.GI5500@mwanda>
Date:	Mon, 16 Jun 2014 22:01:14 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
	Monam Agarwal <monamagarwal123@...il.com>
Subject: Re: [PATCH] staging: ced1401: userspace: use1401.c: Cleaning up
 potential strncpy missing null-terminate

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@...ctrumdigital.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

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