From patchwork Wed Apr 27 12:03:16 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 735611 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter1.kernel.org (8.14.4/8.14.3) with ESMTP id p3RCfO6B007265 for ; Wed, 27 Apr 2011 12:51:44 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758783Ab1D0MDY (ORCPT ); Wed, 27 Apr 2011 08:03:24 -0400 Received: from mail-yx0-f174.google.com ([209.85.213.174]:53097 "EHLO mail-yx0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756147Ab1D0MDX (ORCPT ); Wed, 27 Apr 2011 08:03:23 -0400 Received: by yxs7 with SMTP id 7so399541yxs.19 for ; Wed, 27 Apr 2011 05:03:22 -0700 (PDT) Received: by 10.151.13.7 with SMTP id q7mr1790094ybi.304.1303905802470; Wed, 27 Apr 2011 05:03:22 -0700 (PDT) Received: from salusa.poochiereds.net (cpe-075-177-180-210.nc.res.rr.com [75.177.180.210]) by mx.google.com with ESMTPS id d36sm728037and.30.2011.04.27.05.03.21 (version=SSLv3 cipher=OTHER); Wed, 27 Apr 2011 05:03:21 -0700 (PDT) From: Jeff Layton To: smfrench@gmail.com Cc: linux-cifs@vger.kernel.org, dhowells@redhat.com Subject: [PATCH 3/5] cifs: sanitize length checking in coalesce_t2 (try #2) Date: Wed, 27 Apr 2011 08:03:16 -0400 Message-Id: <1303905796-28087-1-git-send-email-jlayton@redhat.com> X-Mailer: git-send-email 1.7.4.4 In-Reply-To: <17747.1303828052@redhat.com> References: <17747.1303828052@redhat.com> Sender: linux-cifs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-cifs@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Wed, 27 Apr 2011 12:51:44 +0000 (UTC) There are a couple of places in this code where these values can wrap or go negative, and that could potentially end up overflowing the buffer. Ensure that that doesn't happen. Do all of the length calculation and checks first, and only perform the memcpy after they pass. Also, increase some stack variables to 32 bits to ensure that they don't wrap without being detected. Finally, change the error codes to be a bit more descriptive of any problems detected. -EINVAL isn't very accurate. Cc: stable@kernel.org Reported-by: David Howells Signed-off-by: Jeff Layton --- fs/cifs/connect.c | 23 +++++++++++++++++------ 1 files changed, 17 insertions(+), 6 deletions(-) diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index db9d55b..77031a3 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -274,7 +274,8 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB) char *data_area_of_target; char *data_area_of_buf2; int remaining; - __u16 byte_count, total_data_size, total_in_buf, total_in_buf2; + unsigned int byte_count, total_in_buf; + __u16 total_data_size, total_in_buf2; total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount); @@ -287,7 +288,7 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB) remaining = total_data_size - total_in_buf; if (remaining < 0) - return -EINVAL; + return -EPROTO; if (remaining == 0) /* nothing to do, ignore */ return 0; @@ -308,20 +309,30 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB) data_area_of_target += total_in_buf; /* copy second buffer into end of first buffer */ - memcpy(data_area_of_target, data_area_of_buf2, total_in_buf2); total_in_buf += total_in_buf2; + + /* is the result too big for the field? */ + if (total_in_buf & USHRT_MAX) + return -EPROTO; put_unaligned_le16(total_in_buf, &pSMBt->t2_rsp.DataCount); + byte_count = get_bcc_le(pTargetSMB); byte_count += total_in_buf2; + + /* is the result too big for the field? */ + if (byte_count & USHRT_MAX) + return -EPROTO; put_bcc_le(byte_count, pTargetSMB); byte_count = pTargetSMB->smb_buf_length; byte_count += total_in_buf2; - - /* BB also add check that we are not beyond maximum buffer size */ - + /* don't allow buffer to overflow */ + if (byte_count > CIFSMaxBufSize) + return -ENOBUFS; pTargetSMB->smb_buf_length = byte_count; + memcpy(data_area_of_target, data_area_of_buf2, total_in_buf2); + if (remaining == total_in_buf2) { cFYI(1, "found the last secondary response"); return 0; /* we are done */