2010-07-29 15:56:06

by Kulikov Vasiliy

Subject: [PATCH] char: ip2: check put_user() result

put_user()/copy_to_user() may fail, if so return -EFAULT.

Signed-off-by: Kulikov Vasiliy <[email protected]>
---
drivers/char/ip2/ip2main.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/char/ip2/ip2main.c b/drivers/char/ip2/ip2main.c
index 07f3ea3..3f70a05 100644
--- a/drivers/char/ip2/ip2main.c
+++ b/drivers/char/ip2/ip2main.c
@@ -2813,9 +2813,9 @@ DumpTraceBuffer ( char __user *pData, int count )
return -EIO;
}
rc = put_user(tracewrap, pIndex );
- rc = put_user(TRACEMAX, ++pIndex );
- rc = put_user(tracestrip, ++pIndex );
- rc = put_user(tracestuff, ++pIndex );
+ rc |= put_user(TRACEMAX, ++pIndex);
+ rc |= put_user(tracestrip, ++pIndex);
+ rc |= put_user(tracestuff, ++pIndex);
pData += sizeof(int) * 6;
count -= sizeof(int) * 6;

@@ -2828,7 +2828,7 @@ DumpTraceBuffer ( char __user *pData, int count )
}
chunk = TRACEMAX - tracestrip;
if ( dumpcount > chunk ) {
- rc = copy_to_user(pData, &tracebuf[tracestrip],
+ rc |= copy_to_user(pData, &tracebuf[tracestrip],
chunk * sizeof(tracebuf[0]) );
pData += chunk * sizeof(tracebuf[0]);
tracestrip = 0;
@@ -2836,15 +2836,15 @@ DumpTraceBuffer ( char __user *pData, int count )
} else {
chunk = dumpcount;
}
- rc = copy_to_user(pData, &tracebuf[tracestrip],
+ rc |= copy_to_user(pData, &tracebuf[tracestrip],
chunk * sizeof(tracebuf[0]) );
tracestrip += chunk;
tracewrap = 0;

- rc = put_user(tracestrip, ++pIndex );
- rc = put_user(tracestuff, ++pIndex );
+ rc |= put_user(tracestrip, ++pIndex);
+ rc |= put_user(tracestuff, ++pIndex);

- return dumpcount;
+ return rc ? -EFAULT : dumpcount;
#else
return 0;
#endif
--
1.7.0.4

2010-07-29 16:01:29

by Alan

Subject: Re: [PATCH] char: ip2: check put_user() result

O> - return dumpcount;
> + return rc ? -EFAULT : dumpcount;

This is actually at least as wrong as before.

The standards say that if you successfully return some data you should
report the bytes returned. Normally it doesn't matter much because the
data has not been 'lost' when it gets called again.

If you actually remove the data from the queue it ought to be getting
reported with a suitable length.

Alan