From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756076Ab2ADQ3j (ORCPT ); Wed, 4 Jan 2012 11:29:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48298 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751920Ab2ADQ3g convert rfc822-to-8bit (ORCPT ); Wed, 4 Jan 2012 11:29:36 -0500 Date: Wed, 4 Jan 2012 11:29:12 -0500 From: Jeff Layton To: Steve French Cc: Shirish Pargaonkar , linux-kernel@vger.kernel.org, k.skarlatos@gmail.com, linux-cifs@vger.kernel.org Subject: Re: [PATCH] cifs: fix bad buffer length check in coalesce_t2 Message-ID: <20120104112912.139f9578@tlielax.poochiereds.net> In-Reply-To: References: <1325432079-21179-1-git-send-email-jlayton@redhat.com> <20120104090910.7ed8ff12@tlielax.poochiereds.net> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 4 Jan 2012 10:14:25 -0600 Steve French wrote: > FYI - the patch is merged into cifs git tree but want to wait 24 hours > for linux-next before sending merge request upstream. > I wouldn't wait. The 3.2 release is imminent. > On Wed, Jan 4, 2012 at 8:09 AM, Jeff Layton wrote: > > On Tue, 3 Jan 2012 16:46:15 -0600 > > Shirish Pargaonkar wrote: > > > >> On Sun, Jan 1, 2012 at 9:34 AM, Jeff Layton wrote: > >> > The current check looks to see if the RFC1002 length is larger than > >> > CIFSMaxBufSize, and fails if it is. The buffer is actually larger than > >> > that by MAX_CIFS_HDR_SIZE. > >> > > >> > This bug has been around for a long time, but the fact that we used to > >> > cap the clients MaxBufferSize at the same level as the server tended > >> > to paper over it. Commit c974befa changed that however and caused this > >> > bug to bite in more cases. > >> > > >> > Reported-and-Tested-by: Konstantinos Skarlatos > >> > Signed-off-by: Jeff Layton > >> > --- > >> >  fs/cifs/connect.c |    2 +- > >> >  1 files changed, 1 insertions(+), 1 deletions(-) > >> > > >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > >> > index 8cd4b52..27c4f25 100644 > >> > --- a/fs/cifs/connect.c > >> > +++ b/fs/cifs/connect.c > >> > @@ -282,7 +282,7 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB) > >> >        byte_count = be32_to_cpu(pTargetSMB->smb_buf_length); > >> >        byte_count += total_in_buf2; > >> >        /* don't allow buffer to overflow */ > >> > -       if (byte_count > CIFSMaxBufSize) > >> > +       if (byte_count > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) > >> >                return -ENOBUFS; > >> >        pTargetSMB->smb_buf_length = cpu_to_be32(byte_count); > >> > > >> > -- > >> > 1.7.7.4 > >> > > >> > >> This looks correct.  But the way Windows XP server responds to request > >>  trans2/find_first2/info level is different than how Windows 2003 Server > >> and Windows 2008 Server respond. > >> > >> So a related concern would be, for a response from Windows XP server, > >> this check in function check2ndT2 does not make sense. > >>  if (total_data_size > CIFSMaxBufSize) { > >> > > > > This check looks redundant to me. We could remove it since the check in > > coalesce_t2 is more accurate... > > > >> It is possible to have large number of entries in a directory such that the > >> response to a  ls  command can exceed CIFSMaxBufSize. > > > > It shouldn't be possible. The CIFSFindFirst request sends this: > > > >        pSMB->MaxDataCount = cpu_to_le16(CIFSMaxBufSize & 0xFFFFFF00); > > > > ...which should ensure that the amount of data in the response is less > > than CIFSMaxBufSize. I'm not sure what the point of the mask is there > > however... > > > > In addition, we're also limited by this: > > > >        pSMB->SearchCount = cpu_to_le16(CIFSMaxBufSize/sizeof(FILE_UNIX_INFO)); > > > > ...but I think we ought to consider just setting that to 0xffff. > > > > Dividing by sizeof(FILE_UNIX_INFO) is clearly wrong for other > > infolevels. We don't really care how many entries the server sends as > > long as it doesn't exceed the buffer size. > > > > Either way, I believe this patch is correct, though we may have some > > other cleanup work to do in this area. > > > > -- > > Jeff Layton > > > -- Jeff Layton