Re: [patch 1/3] copy_to_user check in fs/cifs/file.c

[Jesper Juhl <juhl-lkml@dif.dk>]

On Sun, 26 Dec 2004, Alan Cox wrote:

> On Sul, 2004-12-26 at 23:24, Jesper Juhl wrote:
> > Hi,
> > 
> > Patch below adds a check for the copy_to_user return value and makes a few 
> > whitespace cleanups in  fs/cifs/file.c::cifs_user_read()
> > I hope bundling two different things together in one patch is OK when the 
> > change is as small as this, but if you want it spplit in two patches, then 
> > just say so.
> 
> Corrupts the stats
> Fails to free smb_read_data where in some cases it was freed before
> 
> I'm not sure the stats matter but I think you need something more like
> 
Like this patch perhaps? :

It implements what you sugested except it breaks the loop when there's 
some residue from the copy_to_user call, since the logic after the while 
loop handles the   return total_read ? total_read : -EFAULT;  case if we 
set rc and break (I've cut the diff with a few lines of extra context to 
make this easy to verify).

Comments welcome as always.


Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

diff --unified=8 -p linux-2.6.10-orig/fs/cifs/file.c linux-2.6.10/fs/cifs/file.c
--- linux-2.6.10-orig/fs/cifs/file.c	2004-12-24 22:33:48.000000000 +0100
+++ linux-2.6.10/fs/cifs/file.c	2004-12-28 23:27:17.000000000 +0100
@@ -1142,16 +1142,17 @@ cifs_user_read(struct file * file, char 
 	open_file = (struct cifsFileInfo *)file->private_data;
 
 	if((file->f_flags & O_ACCMODE) == O_WRONLY) {
 		cFYI(1,("attempting read on write only file instance"));
 	}
 
 	for (total_read = 0,current_offset=read_data; read_size > total_read;
 				total_read += bytes_read,current_offset+=bytes_read) {
+		unsigned residue;
 		current_read_size = min_t(const int,read_size - total_read,cifs_sb->rsize);
 		rc = -EAGAIN;
 		smb_read_data = NULL;
 		while(rc == -EAGAIN) {
 			if ((open_file->invalidHandle) && (!open_file->closePend)) {
 				rc = cifs_reopen_file(file->f_dentry->d_inode,
 					file,TRUE);
 				if(rc != 0)
@@ -1159,22 +1160,27 @@ cifs_user_read(struct file * file, char 
 			}
 
 			rc = CIFSSMBRead(xid, pTcon,
 				 open_file->netfid,
 				 current_read_size, *poffset,
 				 &bytes_read, &smb_read_data);
 
 			pSMBr = (struct smb_com_read_rsp *)smb_read_data;
-			copy_to_user(current_offset,smb_read_data + 4/* RFC1001 hdr*/
+			residue = copy_to_user(current_offset, smb_read_data + 4 /* RFC1001 hdr */
 				+ le16_to_cpu(pSMBr->DataOffset), bytes_read);
 			if(smb_read_data) {
 				cifs_buf_release(smb_read_data);
 				smb_read_data = NULL;
 			}
+			if (residue) {
+				total_read += bytes_read - residue;
+				rc = -EFAULT;
+				break;
+			}
 		}
 		if (rc || (bytes_read == 0)) {
 			if (total_read) {
 				break;
 			} else {
 				FreeXid(xid);
 				return rc;
 			}