----- On Sep 3, 2019, at 12:12 PM, Linus Torvalds [email protected] wrote:
> On Tue, Sep 3, 2019 at 9:00 AM Mathieu Desnoyers > <[email protected]> wrote: >> >> probe_kernel_address can return -EFAULT on error, which leads to use of >> an uninitialized or partially initialized sighand variable. > > I think this comment and this code is actively misleading. > > There is no "uninitialized or partially initialized sighand variable". > That's completely wrong. > > The sighand variable is always completely initialized. It's just that > the check for "is it initialized" is _not_ the return value from > probe_kernel_address(), because that return value is simply not > sufficient. > > So this is just wrong. Don't do it. You're just confusing the issue, > and you're making statments that aren't true in the commit message, > and making the code do a pointless and odd check. > > If you want to change this code for legibility, you should just add a > comment above the probe_kernel_address() about why the return value is > ignored, and why the check _below_ that code verifies the value of > sighand with a different check. Then I must be misunderstanding something. probe_kernel_address() is a macro wrapping probe_kernel_read(). mm/maccess.c:probe_kernel_read() calls probe_read_common() mm/maccess.c:probe_read_common() calls __copy_from_user_inatomic() include/linux/uaccess.h:__copy_from_user_inatomic() documents: * NOTE: only copy_from_user() zero-pads the destination in case of short copy. * Neither __copy_from_user() nor __copy_from_user_inatomic() zero anything * at all; their callers absolutely must check the return value. So considering that comment, I suspect the on-stack sighand variable within task_rcu_dereference() can be left either uninitialized or (less likely) partially initialized if probe_kernel_address() returns -EFAULT. Is there anything else that prevents probe_kernel_address from failing ? If so, why use probe_kernel_address in the first place ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
