| Message ID | 49EDBB2B.8020503@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 21 Apr 2009 17:55:15 +0530 Suresh Jayaraman <sjayaraman@suse.de> wrote: > Make cifs_strlcpy_to_host use UniStrnlenBytes(). Also fix a bug > introduced by a previous patch. The previous patch while fixing the > buffer size, left out the NULL termination that is based on length of > src buffer(in UCS characters) as it is. Fix this by using the length of > dst buffer got from cifs_strfromUCS_le() for NULL termination. > > > Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de> > --- > fs/cifs/cifssmb.c | 29 +++++++++++++++++------------ > 1 files changed, 17 insertions(+), 12 deletions(-) > > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > index a02c43b..aab1d32 100644 > --- a/fs/cifs/cifssmb.c > +++ b/fs/cifs/cifssmb.c > @@ -91,26 +91,31 @@ static int > cifs_strlcpy_to_host(char **dst, const char *src, const int maxlen, > const bool is_unicode, const struct nls_table *nls_codepage) > { > - int plen; > + int src_len, dst_len; > + size_t nbytes; > > if (is_unicode) { > - plen = UniStrnlen((wchar_t *)src, maxlen); > - *dst = kmalloc((4 * plen) + 2, GFP_KERNEL); > + nbytes = UniStrnlenBytes((wchar_t *)src, maxlen, &src_len, > + nls_codepage); > + *dst = kmalloc(nbytes + 2, GFP_KERNEL); > if (!*dst) > - goto cifs_strlcpy_to_host_ErrExit; > - cifs_strfromUCS_le(*dst, (__le16 *)src, plen, nls_codepage); > - (*dst)[plen] = 0; > - (*dst)[plen+1] = 0; /* needed for Unicode */ > + goto err_exit; > + dst_len = cifs_strfromUCS_le(*dst, (__le16 *)src, src_len, > + nls_codepage); > + /* > + * cifs_strfromUCS_le() ensures single byte NULL termination > + */ > + (*dst)[dst_len + 1] = 0; /* needed for Unicode, to be safe */ This is fine for now, but maybe we should just fix cifs_strfromUCS_le to do the double-byte termination? Is a single-byte terminator ever useful? Fixing that will of course mean auditing all of the callers, but I think they all have big enough buffers for this now. Right? > } else { > - plen = strnlen(src, maxlen); > - *dst = kmalloc(plen + 2, GFP_KERNEL); > + src_len = strnlen(src, maxlen); > + *dst = kmalloc(src_len + 1, GFP_KERNEL); > if (!*dst) > - goto cifs_strlcpy_to_host_ErrExit; > - strlcpy(*dst, src, plen); > + goto err_exit; > + strlcpy(*dst, src, src_len); ^^^^^^^ should be src_len+1. > } > return 0; > > -cifs_strlcpy_to_host_ErrExit: > +err_exit: > cERROR(1, ("Failed to allocate buffer for string\n")); > return -ENOMEM; > } > _______________________________________________ > linux-cifs-client mailing list > linux-cifs-client@lists.samba.org > https://lists.samba.org/mailman/listinfo/linux-cifs-client >
On Tue, Apr 21, 2009 at 11:15 AM, Jeff Layton <jlayton@redhat.com> wrote: > On Tue, 21 Apr 2009 17:55:15 +0530 > Suresh Jayaraman <sjayaraman@suse.de> wrote: > >> Make cifs_strlcpy_to_host use UniStrnlenBytes(). Also fix a bug >> introduced by a previous patch. The previous patch while fixing the >> buffer size, left out the NULL termination that is based on length of >> src buffer(in UCS characters) as it is. Fix this by using the length of >> dst buffer got from cifs_strfromUCS_le() for NULL termination. >> index a02c43b..aab1d32 100644 >> --- a/fs/cifs/cifssmb.c >> +++ b/fs/cifs/cifssmb.c >> @@ -91,26 +91,31 @@ static int >> Â cifs_strlcpy_to_host(char **dst, const char *src, const int maxlen, >> Â Â Â Â Â Â Â Â const bool is_unicode, const struct nls_table *nls_codepage) >> Â { >> - Â Â int plen; >> + Â Â int src_len, dst_len; >> + Â Â size_t nbytes; >> >> Â Â Â if (is_unicode) { >> - Â Â Â Â Â Â plen = UniStrnlen((wchar_t *)src, maxlen); >> - Â Â Â Â Â Â *dst = kmalloc((4 * plen) + 2, GFP_KERNEL); >> + Â Â Â Â Â Â nbytes = UniStrnlenBytes((wchar_t *)src, maxlen, &src_len, >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â nls_codepage); >> + Â Â Â Â Â Â *dst = kmalloc(nbytes + 2, GFP_KERNEL); >> Â Â Â Â Â Â Â if (!*dst) >> - Â Â Â Â Â Â Â Â Â Â goto cifs_strlcpy_to_host_ErrExit; >> - Â Â Â Â Â Â cifs_strfromUCS_le(*dst, (__le16 *)src, plen, nls_codepage); >> - Â Â Â Â Â Â (*dst)[plen] = 0; >> - Â Â Â Â Â Â (*dst)[plen+1] = 0; /* needed for Unicode */ >> + Â Â Â Â Â Â Â Â Â Â goto err_exit; >> + Â Â Â Â Â Â dst_len = cifs_strfromUCS_le(*dst, (__le16 *)src, src_len, >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â nls_codepage); >> + Â Â Â Â Â Â /* >> + Â Â Â Â Â Â Â * cifs_strfromUCS_le() ensures single byte NULL termination >> + Â Â Â Â Â Â Â */ >> + Â Â Â Â Â Â (*dst)[dst_len + 1] = 0; /* needed for Unicode, to be safe */ > > This is fine for now, but maybe we should just fix cifs_strfromUCS_le > to do the double-byte termination? Is a single-byte terminator ever > useful? > > Fixing that will of course mean auditing all of the callers, but I > think they all have big enough buffers for this now. Right? Jeff's argument makes sense :) >> Â Â Â } else { >> - Â Â Â Â Â Â plen = strnlen(src, maxlen); >> - Â Â Â Â Â Â *dst = kmalloc(plen + 2, GFP_KERNEL); >> + Â Â Â Â Â Â src_len = strnlen(src, maxlen); >> + Â Â Â Â Â Â *dst = kmalloc(src_len + 1, GFP_KERNEL); >> Â Â Â Â Â Â Â if (!*dst) >> - Â Â Â Â Â Â Â Â Â Â goto cifs_strlcpy_to_host_ErrExit; >> - Â Â Â Â Â Â strlcpy(*dst, src, plen); >> + Â Â Â Â Â Â Â Â Â Â goto err_exit; >> + Â Â Â Â Â Â strlcpy(*dst, src, src_len); > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ^^^^^^^ > should be src_len+1. > >> Â Â Â } >> Â Â Â return 0; >> >> -cifs_strlcpy_to_host_ErrExit: >> +err_exit: >> Â Â Â cERROR(1, ("Failed to allocate buffer for string\n")); >> Â Â Â return -ENOMEM; >> Â } >> _______________________________________________ >> linux-cifs-client mailing list >> linux-cifs-client@lists.samba.org >> https://lists.samba.org/mailman/listinfo/linux-cifs-client >> > > > -- > Jeff Layton <jlayton@redhat.com> >
Jeff Layton wrote: > On Tue, 21 Apr 2009 17:55:15 +0530 > Suresh Jayaraman <sjayaraman@suse.de> wrote: > >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c >> index a02c43b..aab1d32 100644 >> --- a/fs/cifs/cifssmb.c >> +++ b/fs/cifs/cifssmb.c >> @@ -91,26 +91,31 @@ static int >> cifs_strlcpy_to_host(char **dst, const char *src, const int maxlen, >> const bool is_unicode, const struct nls_table *nls_codepage) >> { >> - int plen; >> + int src_len, dst_len; >> + size_t nbytes; >> >> if (is_unicode) { >> - plen = UniStrnlen((wchar_t *)src, maxlen); >> - *dst = kmalloc((4 * plen) + 2, GFP_KERNEL); >> + nbytes = UniStrnlenBytes((wchar_t *)src, maxlen, &src_len, >> + nls_codepage); >> + *dst = kmalloc(nbytes + 2, GFP_KERNEL); >> if (!*dst) >> - goto cifs_strlcpy_to_host_ErrExit; >> - cifs_strfromUCS_le(*dst, (__le16 *)src, plen, nls_codepage); >> - (*dst)[plen] = 0; >> - (*dst)[plen+1] = 0; /* needed for Unicode */ >> + goto err_exit; >> + dst_len = cifs_strfromUCS_le(*dst, (__le16 *)src, src_len, >> + nls_codepage); >> + /* >> + * cifs_strfromUCS_le() ensures single byte NULL termination >> + */ >> + (*dst)[dst_len + 1] = 0; /* needed for Unicode, to be safe */ > > This is fine for now, but maybe we should just fix cifs_strfromUCS_le Yes, I think. > to do the double-byte termination? Is a single-byte terminator ever > useful? I just looked at all the callers (15) and there is no caller where a single byte terminator is useful. There are couple of callers (mostly unused SessSetup/NTLMSSP code plus CIFSTCon) where this would not matter as they already use kzalloc(). > Fixing that will of course mean auditing all of the callers, but I > think they all have big enough buffers for this now. Right? Most of them have sufficient buffers. The following ones needs attention: - symlinkinfo buffer in CIFSSMBUnixQuerySymLink()(allocated in cifs_follow_link) - Is PATH_MAX (4096) the MAX for nls strings too? or may be we need to move the allocation to CIFSSMBUnixQuerySymLink() and use UniStrnlenBytes? - CIFSSMBQueryReparseLinkInfo() (allocated in cifs_readlink() which is unused anyway. Not sure whether it will be used in future or not). - buffers in SessSetup/NTLMSP code. Do we need to fix this too? It appears to me that we can fix cifs_strfromUCS_le to do double-byte NULL termination as most of the in-use buffers are big enough. >> } else { >> - plen = strnlen(src, maxlen); >> - *dst = kmalloc(plen + 2, GFP_KERNEL); >> + src_len = strnlen(src, maxlen); >> + *dst = kmalloc(src_len + 1, GFP_KERNEL); >> if (!*dst) >> - goto cifs_strlcpy_to_host_ErrExit; >> - strlcpy(*dst, src, plen); >> + goto err_exit; >> + strlcpy(*dst, src, src_len); > ^^^^^^^ > should be src_len+1. Yes, should be fixed. >> } >> return 0; >> >> -cifs_strlcpy_to_host_ErrExit: >> +err_exit: >> cERROR(1, ("Failed to allocate buffer for string\n")); >> return -ENOMEM; >> } >> _______________________________________________
On Wed, 22 Apr 2009 15:25:50 +0530 Suresh Jayaraman <sjayaraman@suse.de> wrote: > > This is fine for now, but maybe we should just fix cifs_strfromUCS_le > > Yes, I think. > > > to do the double-byte termination? Is a single-byte terminator ever > > useful? > > I just looked at all the callers (15) and there is no caller where a > single byte terminator is useful. There are couple of callers (mostly > unused SessSetup/NTLMSSP code plus CIFSTCon) where this would not matter > as they already use kzalloc(). > > > Fixing that will of course mean auditing all of the callers, but I > > think they all have big enough buffers for this now. Right? > > Most of them have sufficient buffers. The following ones needs attention: > > - symlinkinfo buffer in CIFSSMBUnixQuerySymLink()(allocated in > cifs_follow_link) - Is PATH_MAX (4096) the MAX for nls strings too? > or may be we need to move the allocation to CIFSSMBUnixQuerySymLink() > and use UniStrnlenBytes? > That's probably the best solution -- just switch it to use strlcpy_to_host. Let's use that helper wherever we can (unless doing so is problematic for some reason). > - CIFSSMBQueryReparseLinkInfo() (allocated in cifs_readlink() which is > unused anyway. Not sure whether it will be used in future or not). > You're right that that's unused. Steve, care to comment? What was the intent of that code? > - buffers in SessSetup/NTLMSP code. Do we need to fix this too? > That code is heavily bitrotted now. Maybe we should just rip it out. If we end up taking something like that patchset I proposed yesterday it'll also lay the groundwork for doing NTLMSSP in cifs.upcall. We can use the new "credinfo" arg to send the password to the upcall. It's hard to imagine that anyone is setting the experimental flag in /proc/fs/cifs in order to use it. > It appears to me that we can fix cifs_strfromUCS_le to do double-byte > NULL termination as most of the in-use buffers are big enough. > I suggest that you go ahead and do it then. > >> } else { > >> - plen = strnlen(src, maxlen); > >> - *dst = kmalloc(plen + 2, GFP_KERNEL); > >> + src_len = strnlen(src, maxlen); > >> + *dst = kmalloc(src_len + 1, GFP_KERNEL); > >> if (!*dst) > >> - goto cifs_strlcpy_to_host_ErrExit; > >> - strlcpy(*dst, src, plen); > >> + goto err_exit; > >> + strlcpy(*dst, src, src_len); > > ^^^^^^^ > > should be src_len+1. > > Yes, should be fixed. > > >> } > >> return 0; > >> > >> -cifs_strlcpy_to_host_ErrExit: > >> +err_exit: > >> cERROR(1, ("Failed to allocate buffer for string\n")); > >> return -ENOMEM; > >> } > >> _______________________________________________ > > > > -- > Suresh Jayaraman
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index a02c43b..aab1d32 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -91,26 +91,31 @@ static int cifs_strlcpy_to_host(char **dst, const char *src, const int maxlen, const bool is_unicode, const struct nls_table *nls_codepage) { - int plen; + int src_len, dst_len; + size_t nbytes; if (is_unicode) { - plen = UniStrnlen((wchar_t *)src, maxlen); - *dst = kmalloc((4 * plen) + 2, GFP_KERNEL); + nbytes = UniStrnlenBytes((wchar_t *)src, maxlen, &src_len, + nls_codepage); + *dst = kmalloc(nbytes + 2, GFP_KERNEL); if (!*dst) - goto cifs_strlcpy_to_host_ErrExit; - cifs_strfromUCS_le(*dst, (__le16 *)src, plen, nls_codepage); - (*dst)[plen] = 0; - (*dst)[plen+1] = 0; /* needed for Unicode */ + goto err_exit; + dst_len = cifs_strfromUCS_le(*dst, (__le16 *)src, src_len, + nls_codepage); + /* + * cifs_strfromUCS_le() ensures single byte NULL termination + */ + (*dst)[dst_len + 1] = 0; /* needed for Unicode, to be safe */ } else { - plen = strnlen(src, maxlen); - *dst = kmalloc(plen + 2, GFP_KERNEL); + src_len = strnlen(src, maxlen); + *dst = kmalloc(src_len + 1, GFP_KERNEL); if (!*dst) - goto cifs_strlcpy_to_host_ErrExit; - strlcpy(*dst, src, plen); + goto err_exit; + strlcpy(*dst, src, src_len); } return 0; -cifs_strlcpy_to_host_ErrExit: +err_exit: cERROR(1, ("Failed to allocate buffer for string\n")); return -ENOMEM; }
Make cifs_strlcpy_to_host use UniStrnlenBytes(). Also fix a bug introduced by a previous patch. The previous patch while fixing the buffer size, left out the NULL termination that is based on length of src buffer(in UCS characters) as it is. Fix this by using the length of dst buffer got from cifs_strfromUCS_le() for NULL termination. Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de> --- fs/cifs/cifssmb.c | 29 +++++++++++++++++------------ 1 files changed, 17 insertions(+), 12 deletions(-)