On Tue, 7 Sep 2010 07:52:09 -0500
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Tue, Sep 7, 2010 at 6:56 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> > On Mon, 6 Sep 2010 22:34:57 -0500
> > shirishpargaonkar@xxxxxxxxx wrote:
> >
> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >>
> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >> ---
> >> fs/cifs/cifsencrypt.c | 68 +++++++++++++++++++++++++++++++++++++++++++++---
> >> fs/cifs/cifspdu.h | 1 -
> >> fs/cifs/cifsproto.h | 2 +-
> >> fs/cifs/connect.c | 2 +
> >> fs/cifs/sess.c | 24 ++++++++++++++++-
> >> 5 files changed, 88 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> >> index fe1e4c9..5f0fc6b 100644
> >> --- a/fs/cifs/cifsencrypt.c
> >> +++ b/fs/cifs/cifsencrypt.c
> >> @@ -27,6 +27,7 @@
> >> #include "md5.h"
> >> #include "cifs_unicode.h"
> >> #include "cifsproto.h"
> >> +#include "ntlmssp.h"
> >> #include <linux/ctype.h>
> >> #include <linux/random.h>
> >>
> >> @@ -263,6 +264,56 @@ void calc_lanman_hash(const char *password, const char *cryptkey, bool encrypt,
> >> }
> >> #endif /* CIFS_WEAK_PW_HASH */
> >>
> >> +static int
> >> +find_domain_name(struct cifsSesInfo *ses)
> >> +{
> >> + int rc = 0;
> >> + unsigned int attrsize;
> >> + unsigned int type;
> >> + unsigned char *blobptr;
> >> + struct ntlmssp2_name *attrptr;
> >> +
> >> + if (ses->server->tiblob) {
> >> + blobptr = ses->server->tiblob;
> >> + attrptr = (struct ntlmssp2_name *) blobptr;
> >> +
> >> + while ((type = attrptr->type) != 0) {
> >> + blobptr += 2; /* advance attr type */
> >> + attrsize = attrptr->length;
> >> + blobptr += 2; /* advance attr size */
> >> + if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
> >> + if (!ses->domainName) {
> >> + ses->domainName =
> >> + kmalloc(attrptr->length + 1,
> >> + GFP_KERNEL);
> >> + if (!ses->domainName)
> >> + return -ENOMEM;
> >> + cifs_from_ucs2(ses->domainName,
> >> + (__le16 *)blobptr,
> >> + attrptr->length,
> >> + attrptr->length,
> >> + load_nls_default(), false);
> > ^^^^^^^^
> > Doesn't
> > "load_nls_default" need
> > an "unload_nls" when
> > you're done with it?
> >> + }
> >> + }
> >> + blobptr += attrsize; /* advance attr value */
> >> + attrptr = (struct ntlmssp2_name *) blobptr;
> >> + }
> >> + } else {
> >> + ses->server->tilen = 2 * sizeof(struct ntlmssp2_name);
> >> + ses->server->tiblob = kmalloc(ses->server->tilen, GFP_KERNEL);
> >> + if (!ses->server->tiblob) {
> >> + ses->server->tilen = 0;
> >> + cERROR(1, "Challenge target info allocation failure");
> >> + return -ENOMEM;
> >> + }
> >> + memset(ses->server->tiblob, 0x0, ses->server->tilen);
> > ^^^^^^^^^^
> > This memset can be eliminated if you use kzalloc
> > instead.
>
> OK, will change the call.
>
> >
> >> + attrptr = (struct ntlmssp2_name *) ses->server->tiblob;
> >> + attrptr->type = cpu_to_le16(NTLMSSP_DOMAIN_TYPE);
> >> + }
> >> +
> >> + return rc;
> >> +}
> >> +
> >
> > A comment describing what the above function is intended to do would be
> > nice. I can sort of guess that it's intended to parse the domain name
> > out of one of the NTLMSSP blobs, but I don't quite understand what the
> > stuff in the big "else" clause is for. Why allocate the buffer for the
> > tiblob in "find_domain_name" ? It seems like that ought to be done in a
> > more well-defined fashion.
>
> Because av pairs were removed from struct ntlmv2_resp, if we are using a
> a non ntlmssp sec mech e.g. sec=ntlmv2/ntlmv2i, the very same tiblob is
> created as it was done before. Servers like Windows 2003 are not very
> particular about this blob and so this blob does not need all the Target Info/
> AV pairs such as dns/netbios names of the domain and server, timestamp etc.
> But you need that (else part) blob nonetheless for sec=ntlmv2i like of security
> mech options.
>
This seems like a red flag for the design of this whole thing. This
buffer ought not be allocated in find_domain_name(). One reason is that
it's not at all obvious why a function like find_domain_name ought to
be responsible for allocating something unrelated to the domain name.
These built-in "side effects" for some functions make CIFS really hard
to understand and maintain.
The lifecycle of this the tibuf is very difficult to follow with this
design. As soon as you know the secType and whether signing is enabled,
you'll know whether you need this buffer, right? So why not allocate it
then in a single, well-defined spot?
--
Jeff Layton <jlayton@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
