On Mon, 4 Oct 2010 18:08:24 -0500
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Mon, Oct 4, 2010 at 5:41 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > On Wed, 29 Sep 2010 09:38:48 -0500
> > shirishpargaonkar@xxxxxxxxx wrote:
> >
> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >>
> >>
> >> Fix incorrect calculation of case sensitive response length in the
> >> ntlmv2 (without extended security) response.
> >>
> >>
> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >> ---
> >> fs/cifs/sess.c | 11 +++++++----
> >> 1 files changed, 7 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
> >> index af18a50..9148fd8 100644
> >> --- a/fs/cifs/sess.c
> >> +++ b/fs/cifs/sess.c
> >> @@ -739,9 +739,6 @@ ssetup_ntlmssp_authenticate:
> >> pSMB->req_no_secext.CaseInsensitivePasswordLength = 0;
> >> /* cpu_to_le16(LM2_SESS_KEY_SIZE); */
> >>
> >> - pSMB->req_no_secext.CaseSensitivePasswordLength =
> >> - cpu_to_le16(sizeof(struct ntlmv2_resp));
> >> -
> >> /* calculate session key */
> >> rc = setup_ntlmv2_rsp(ses, v2_sess_key, nls_cp);
> >> if (rc) {
> >> @@ -756,11 +753,17 @@ ssetup_ntlmssp_authenticate:
> >> if (ses->tilen > 0) {
> >> memcpy(bcc_ptr, ses->tiblob, ses->tilen);
> >> bcc_ptr += ses->tilen;
> >> + pSMB->req_no_secext.CaseSensitivePasswordLength =
> >> + cpu_to_le16(sizeof(struct ntlmv2_resp) +
> >> + ses->tilen);
> >> /* we never did allocate ses->domainName to free */
> >> kfree(ses->tiblob);
> >> ses->tiblob = NULL;
> >> ses->tilen = 0;
> >> - }
> >> + } else
> >> + pSMB->req_no_secext.CaseSensitivePasswordLength =
> >> + cpu_to_le16(sizeof(struct ntlmv2_resp));
> >> +
> >> if (ses->capabilities & CAP_UNICODE) {
> >> if (iov[0].iov_len % 2) {
> >> *bcc_ptr = 0;
> >
> > Looks reasonable, but I think it would be clearer to just have a single
> > place that sets CaseSensitivePasswordLength. If ses->tilen > 0, then you
> > add the tilen to it...but tilen will never be less than 0, so there's
> > no need to have this inside that if block, right?
> >
> > --
> > Jeff Layton <jlayton@xxxxxxxxxx>
> >
>
> I think the patch may be fine just the way it is. If the field already
> has little endian value,
> it may be an extra code to convert exiting value, add tilen, and store
> back little endian
> encoded value. That was my original thought behind this code.
I'm not sure I follow your logic. I was just saying that you could move
the setting of CaseSensitivePasswordLength outside of that if block
altogether and eliminate the else clause. That makes the code more
compact and readable.
For instance:
pSMB->req_no_secext.CaseSensitivePasswordLength =
cpu_to_le16(sizeof(struct ntlmv2_resp) + ses->tilen);
if (ses->tilen > 0) {
...etc...
}
The else clause just exists for when ses->tilen == 0, so there's no
need for an else clause there.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
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
