Date:   Sun, 25 Aug 2019 11:59:08 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        syzbot <syzbot+8ab2d0f39fb79fe6ca40@...kaller.appspotmail.com>
Subject: Re: [PATCH] /dev/mem: Bail out upon SIGKILL when reading memory.


* Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp> wrote:

> On 2019/08/25 5:22, Ingo Molnar wrote:
> >> So I'd be willing to try that (and then if somebody reports a
> >> regression we can make it use "fatal_signal_pending()" instead)
> > 
> > Ok, will post a changelogged patch (unless Tetsuo beats me to it?).
> 
> Here is a patch. This patch also tries to fix handling of return code when
> partial read/write happened (because we should return bytes processed when
> we return due to -EINTR). But asymmetric between read function and write
> function looks messy. Maybe we should just make /dev/{mem,kmem} killable
> for now, and defer making /dev/{mem,kmem} interruptible till rewrite of
> read/write functions.
> 
>  drivers/char/mem.c | 89 ++++++++++++++++++++++++++++++------------------------
>  1 file changed, 50 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index cb8e653..3c6a3c2 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -108,7 +108,7 @@ static ssize_t read_mem(struct file *file, char __user *buf,
>  	ssize_t read, sz;
>  	void *ptr;
>  	char *bounce;
> -	int err;
> +	int err = 0;
>  
>  	if (p != *ppos)
>  		return 0;
> @@ -132,8 +132,10 @@ static ssize_t read_mem(struct file *file, char __user *buf,
>  #endif
>  
>  	bounce = kmalloc(PAGE_SIZE, GFP_KERNEL);
> -	if (!bounce)
> -		return -ENOMEM;
> +	if (!bounce) {
> +		err = -ENOMEM;
> +		goto failed;
> +	}

Yeah, so while I agree with the more consistent handling of partial 
reads, I'd suggest the following changes:

 - Please don't use this 4-line error handling variant, use the old short 
   2-line pattern instead. There's no real reason to keep 'err' as a 
   flag, the 'failed' branch will know that 'err' is the error return if 
   there's been no progress.

 - We should probably separate out a third 'fatal error' variant: for 
   example if copying to user-space generates a page fault, then we 
   clearly should not pretend that all is fine and return a short read 
   even if we made some progress, a -EFAULT is more informative, as we 
   might have corrupted (overran) some user buffer on the failed copy as 
   well, and ran off the end into the first unmapped user area.

 - As for the patch series maybe it might make sense to separate the 
   fixes from the semantic changes, in case there's any breakage. I.e. 
   first fix the bug minimally, then add the other changes in a separate 
   commit. If any of them causes problems with applications we'll have a 
   more precise bisection result.

 - Likewise, the changing of the write side interruptability of /dev/mem 
   should probably be a separate patch as well.

I can factor out such a series if you don't have the time, but feel free 
to do it yourself, this is your bug report and your patch. :)

> @@ -180,14 +182,11 @@ static ssize_t read_mem(struct file *file, char __user *buf,
>  		count -= sz;
>  		read += sz;
>  	}
> +failed:
>  	kfree(bounce);
>  
>  	*ppos += read;
> -	return read;
> -
> -failed:
> -	kfree(bounce);
> -	return err;
> +	return read ? read : err;
>  }

Yeah, the merging of the normal and the failure path control flow doesn't 
really help readability and makes the actual iterator more complex - I 
think the old exception handling pattern was fine.

I think the same applies to the write path as well.

Thanks,

	Ingo

Powered by blists - more mailing lists