diff mbox series

proc: Avoid mixing integer types in mem_rw()

Message ID 20210512125215.3348316-1-marcelo.cerri@canonical.com (mailing list archive)
State New, archived
Headers show
Series proc: Avoid mixing integer types in mem_rw() | expand

Commit Message

Marcelo Henrique Cerri May 12, 2021, 12:52 p.m. UTC 
Use size_t when capping the count argument received by mem_rw(). Since
count is size_t, using min_t(int, ...) can lead to a negative value
that will later be passed to access_remote_vm(), which can cause
unexpected behavior.

Since we are capping the value to at maximum PAGE_SIZE, the conversion
from size_t to int when passing it to access_remote_vm() as "len"
shouldn't be a problem.

Reviewed-by: David Disseldorp <ddiss@suse.de>
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
---
 fs/proc/base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alexey Dobriyan May 12, 2021, 1:19 p.m. UTC | #1 
On Wed, May 12, 2021 at 09:52:12AM -0300, Marcelo Henrique Cerri wrote:
> Use size_t when capping the count argument received by mem_rw(). Since
> count is size_t, using min_t(int, ...) can lead to a negative value
> that will later be passed to access_remote_vm(), which can cause
> unexpected behavior.
> 
> Since we are capping the value to at maximum PAGE_SIZE, the conversion
> from size_t to int when passing it to access_remote_vm() as "len"
> shouldn't be a problem.

> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -854,7 +854,7 @@ static ssize_t mem_rw(struct file *file, char __user *buf,
>  	flags = FOLL_FORCE | (write ? FOLL_WRITE : 0);
>  
>  	while (count > 0) {
> -		int this_len = min_t(int, count, PAGE_SIZE);
> +		size_t this_len = min_t(size_t, count, PAGE_SIZE);

As much as I don't like signed integers, VFS caps read/write lengths
at INT_MAX & PAGE_MASK, so casting doesn't change values.
Marcelo Henrique Cerri May 12, 2021, 1:40 p.m. UTC | #2 
On Wed, May 12, 2021 at 04:19:26PM +0300, Alexey Dobriyan wrote:
> On Wed, May 12, 2021 at 09:52:12AM -0300, Marcelo Henrique Cerri wrote:
> > Use size_t when capping the count argument received by mem_rw(). Since
> > count is size_t, using min_t(int, ...) can lead to a negative value
> > that will later be passed to access_remote_vm(), which can cause
> > unexpected behavior.
> > 
> > Since we are capping the value to at maximum PAGE_SIZE, the conversion
> > from size_t to int when passing it to access_remote_vm() as "len"
> > shouldn't be a problem.
> 
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -854,7 +854,7 @@ static ssize_t mem_rw(struct file *file, char __user *buf,
> >  	flags = FOLL_FORCE | (write ? FOLL_WRITE : 0);
> >  
> >  	while (count > 0) {
> > -		int this_len = min_t(int, count, PAGE_SIZE);
> > +		size_t this_len = min_t(size_t, count, PAGE_SIZE);
> 
> As much as I don't like signed integers, VFS caps read/write lengths
> at INT_MAX & PAGE_MASK, so casting doesn't change values.

Although that should always be true we had a recent example of a
caller that wasn't properly capping it and since count is unsigned
anyway it makes sense to keep this value positive which might mitigate
similar issues in the future.
diff mbox series

Patch

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 3851bfcdba56..8dbc6a1aaadb 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -854,7 +854,7 @@  static ssize_t mem_rw(struct file *file, char __user *buf,
 	flags = FOLL_FORCE | (write ? FOLL_WRITE : 0);
 
 	while (count > 0) {
-		int this_len = min_t(int, count, PAGE_SIZE);
+		size_t this_len = min_t(size_t, count, PAGE_SIZE);
 
 		if (write && copy_from_user(page, buf, this_len)) {
 			copied = -EFAULT;