On Tue, Sep 7, 2010 at 8:16 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> 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?
>
yes, will change the code to do that.
> --
> 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
>
--
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
