Skip to contentSkip to conversation

cifs: initialize rsp_iov and avoid a NULL deref in SMB2_read

PATCH In review Ronnie Sahlberg 2 participants 3 messages Oct 24, 2017 – Nov 1, 2017 2 messages not in the archive

Subsystems (2)
SMB client (CIFS) Kernel SMB3 server (ksmbd)
lore
Share & tools
lore

Ronnie Sahlberg

author
#0 lore raw

PATCH 1/1 cifs: initialize rsp_iov and avoid a NULL deref in SMB2_read

Signed-off-by Ronnie Sahlberg <[email protected]>
---
 fs/cifs/smb2pdu.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
fs/cifs/smb2pdu.c +15 -15
index 6ff4c275ca9a..efa06068e7e1 100644
@@ -2669,27 +2669,27 @@ SMB2_read(const unsigned int xid, struct cifs_io_parms *io_parms,
 	cifs_small_buf_release(req);
 
 	rsp = (struct smb2_read_rsp *)rsp_iov.iov_base;
-	shdr = get_sync_hdr(rsp);
 
-	if (shdr->Status == STATUS_END_OF_FILE) {
+	if (rc) {
+		if (rc != -ENODATA) {
+			cifs_stats_fail_inc(io_parms->tcon, SMB2_READ_HE);
+			cifs_dbg(VFS, "Send error in read = %d\n", rc);
+		}
 		free_rsp_buf(resp_buftype, rsp_iov.iov_base);
-		return 0;
+		return rc == -ENODATA ? 0 : rc;
 	}
 
-	if (rc) {
-		cifs_stats_fail_inc(io_parms->tcon, SMB2_READ_HE);
-		cifs_dbg(VFS, "Send error in read = %d\n", rc);
-	} else {
-		*nbytes = le32_to_cpu(rsp->DataLength);
-		if ((*nbytes > CIFS_MAX_MSGSIZE) ||
-		    (*nbytes > io_parms->length)) {
-			cifs_dbg(FYI, "bad length %d for count %d\n",
-				 *nbytes, io_parms->length);
-			rc = -EIO;
-			*nbytes = 0;
-		}
+	*nbytes = le32_to_cpu(rsp->DataLength);
+	if ((*nbytes > CIFS_MAX_MSGSIZE) ||
+	    (*nbytes > io_parms->length)) {
+		cifs_dbg(FYI, "bad length %d for count %d\n",
+			 *nbytes, io_parms->length);
+		rc = -EIO;
+		*nbytes = 0;
 	}
 
+	shdr = get_sync_hdr(rsp);
+
 	if (*buf) {
 		memcpy(*buf, (char *)shdr + rsp->DataOffset, *nbytes);
 		free_rsp_buf(resp_buftype, rsp_iov.iov_base);
Signature · 1 line
2.13.3

A message that is not in the archive (sent off-list or not yet received): 20171024030153.541-1-lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

Reviewed-by
#2 lore raw

2017-10-23 20:01 GMT-07:00 Ronnie Sahlberg <[email protected]>:

Quoted 53 lines
Signed-off-by Ronnie Sahlberg <[email protected]>
---
 fs/cifs/smb2pdu.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
fs/cifs/smb2pdu.c +15 -16
index 6ff4c275ca9a..efa06068e7e1 100644
@@ -2669,27 +2669,27 @@ SMB2_read(const unsigned int xid, struct cifs_io_parms *io_parms,
        cifs_small_buf_release(req);
 
        rsp = (struct smb2_read_rsp *)rsp_iov.iov_base;
-       shdr = get_sync_hdr(rsp);
 
-       if (shdr->Status == STATUS_END_OF_FILE) {
+       if (rc) {
+               if (rc != -ENODATA) {
+                       cifs_stats_fail_inc(io_parms->tcon, SMB2_READ_HE);
+                       cifs_dbg(VFS, "Send error in read = %d\n", rc);
+               }
                free_rsp_buf(resp_buftype, rsp_iov.iov_base);
-               return 0;
+               return rc == -ENODATA ? 0 : rc;
        }
 
-       if (rc) {
-               cifs_stats_fail_inc(io_parms->tcon, SMB2_READ_HE);
-               cifs_dbg(VFS, "Send error in read = %d\n", rc);
-       } else {
-               *nbytes = le32_to_cpu(rsp->DataLength);
-               if ((*nbytes > CIFS_MAX_MSGSIZE) ||
-                   (*nbytes > io_parms->length)) {
-                       cifs_dbg(FYI, "bad length %d for count %d\n",
-                                *nbytes, io_parms->length);
-                       rc = -EIO;
-                       *nbytes = 0;
-               }
+       *nbytes = le32_to_cpu(rsp->DataLength);
+       if ((*nbytes > CIFS_MAX_MSGSIZE) ||
+           (*nbytes > io_parms->length)) {
+               cifs_dbg(FYI, "bad length %d for count %d\n",
+                        *nbytes, io_parms->length);
+               rc = -EIO;
+               *nbytes = 0;
        }
 
+       shdr = get_sync_hdr(rsp);
+
        if (*buf) {
                memcpy(*buf, (char *)shdr + rsp->DataOffset, *nbytes);
                free_rsp_buf(resp_buftype, rsp_iov.iov_base);
--
Quoted 2 lines

2.13.3

Looks good.

Reviewed-by Pavel Shilovsky <[email protected]>

--
Best regards,
Pavel Shilovsky

A message that is not in the archive (sent off-list or not yet received): CAKywueQbQdsE23YE-jJg1t815kBtwFVOfijGxwCnC9Oa0okwhQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#4 lore raw

2017-11-01 9:53 GMT-07:00 Pavel Shilovsky <[email protected]>:

Quoted 59 lines

2017-10-23 20:01 GMT-07:00 Ronnie Sahlberg <[email protected]>:

Quoted 53 lines
Signed-off-by Ronnie Sahlberg <[email protected]>
---
 fs/cifs/smb2pdu.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
fs/cifs/smb2pdu.c +15 -16
index 6ff4c275ca9a..efa06068e7e1 100644
@@ -2669,27 +2669,27 @@ SMB2_read(const unsigned int xid, struct cifs_io_parms *io_parms,
        cifs_small_buf_release(req);
 
        rsp = (struct smb2_read_rsp *)rsp_iov.iov_base;
-       shdr = get_sync_hdr(rsp);
 
-       if (shdr->Status == STATUS_END_OF_FILE) {
+       if (rc) {
+               if (rc != -ENODATA) {
+                       cifs_stats_fail_inc(io_parms->tcon, SMB2_READ_HE);
+                       cifs_dbg(VFS, "Send error in read = %d\n", rc);
+               }
                free_rsp_buf(resp_buftype, rsp_iov.iov_base);
-               return 0;
+               return rc == -ENODATA ? 0 : rc;
        }
 
-       if (rc) {
-               cifs_stats_fail_inc(io_parms->tcon, SMB2_READ_HE);
-               cifs_dbg(VFS, "Send error in read = %d\n", rc);
-       } else {
-               *nbytes = le32_to_cpu(rsp->DataLength);
-               if ((*nbytes > CIFS_MAX_MSGSIZE) ||
-                   (*nbytes > io_parms->length)) {
-                       cifs_dbg(FYI, "bad length %d for count %d\n",
-                                *nbytes, io_parms->length);
-                       rc = -EIO;
-                       *nbytes = 0;
-               }
+       *nbytes = le32_to_cpu(rsp->DataLength);
+       if ((*nbytes > CIFS_MAX_MSGSIZE) ||
+           (*nbytes > io_parms->length)) {
+               cifs_dbg(FYI, "bad length %d for count %d\n",
+                        *nbytes, io_parms->length);
+               rc = -EIO;
+               *nbytes = 0;
        }
 
+       shdr = get_sync_hdr(rsp);
+
        if (*buf) {
                memcpy(*buf, (char *)shdr + rsp->DataOffset, *nbytes);
                free_rsp_buf(resp_buftype, rsp_iov.iov_base);
--
Quoted 2 lines

2.13.3

Looks good.

Quoted 1 line
Reviewed-by Pavel Shilovsky <[email protected]>

It seems like a good stable candidate. Thoughts?

--
Best regards,
Pavel Shilovsky