linux-kernel - Re: [PATCH] /dev/mem: Bail out upon SIGKILL when reading memory.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cfc26ac5-b674-5d42-05f8-c978613aaf29@i-love.sakura.ne.jp>
Date:   Sun, 25 Aug 2019 19:35:16 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
To:     Ingo Molnar <mingo@...nel.org>
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.

On 2019/08/25 18:59, Ingo Molnar wrote:
>> @@ -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.

The caller might guarantee count > 0, but for robustness, I decided to
choose 4-line error handling here because I merged the normal and the
failure path control flow; read will remain 0 if count == 0, and thus
err should remain 0.

> 
>  - 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.

Is it possible that copy_from_user() corrupts user buffer in a way that userspace
cannot retry when kernel responded with "there was a short write"? It seems that
these functions are difficult to return appropriate errors...

> 
>  - 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.

Yes. I think for now we should just make these functions killable. Then, we
can try changing return code for partial read/write. If no breakage report,
we can make these functions interruptible.

> 
>  - 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. :)

You can do it. It's a syzbot's bug report. I just forwarded it. ;-)