From: Eric W. Biederman <hidden> Date: 2020-06-29 20:24:55
Tetsuo Handa [off-list ref] writes:
On 2020/06/29 4:44, Alexei Starovoitov wrote:
quoted
But all the defensive programming kinda goes against general kernel style.
I wouldn't do it. Especially pr_info() ?!
Though I don't feel strongly about it.
Honestly speaking, caller should check for errors and print appropriate
messages. info->wd.mnt->mnt_root != info->wd.dentry indicates that something
went wrong (maybe memory corruption). But other conditions are not fatal.
That is, I consider even pr_info() here should be unnecessary.
They were all should never happen cases. Which is why my patches do:
if (WARN_ON_ONCE(...))
That let's the caller know the messed up very clearly while still
providing a change to continue.
If they were clearly corruption no ones kernel should ever continue
BUG_ON would be appropriate.
quoted
I would like to generalize elf_header_check() a bit and call it
before doing blob_to_mnt() to make sure that all blobs are elf files only.
Supporting '#!/bin/bash' or other things as blobs seems wrong to me.
I vote for not worry about things that have never happened, and are
obviously incorrect.
The only points of checks like that is to catch cases where other
developers misunderstand the interface. When you get to something like
sysfs with lots and lots of users where it is hard to audit there
is real value in sanity checks. In something like this with very few
users. Just making the code clear should be enough for people not to do
ridiculous things.
In any case Tetsuo I will leave futher sanity checks for you and Alexei
to work out. It is beyond the scope of my patchset, and they are easy
enough to add as follow on patches.
Eric
But all the defensive programming kinda goes against general kernel style.
I wouldn't do it. Especially pr_info() ?!
Though I don't feel strongly about it.
Honestly speaking, caller should check for errors and print appropriate
messages. info->wd.mnt->mnt_root != info->wd.dentry indicates that something
went wrong (maybe memory corruption). But other conditions are not fatal.
That is, I consider even pr_info() here should be unnecessary.
They were all should never happen cases. Which is why my patches do:
if (WARN_ON_ONCE(...))
No. Fuzz testing (which uses panic_on_warn=1) will trivially hit them.
This bug was unfortunately not found by syzkaller because this path is
not easily reachable via syscall interface.
That let's the caller know the messed up very clearly while still
providing a change to continue.
If they were clearly corruption no ones kernel should ever continue
BUG_ON would be appropriate.
Please use BUG_ON() (to only corruption case) like I suggested in my updated diff.
On Tue, Jun 30, 2020 at 03:28:49PM +0900, Tetsuo Handa wrote:
On 2020/06/30 5:19, Eric W. Biederman wrote:
quoted
Tetsuo Handa [off-list ref] writes:
quoted
On 2020/06/29 4:44, Alexei Starovoitov wrote:
quoted
But all the defensive programming kinda goes against general kernel style.
I wouldn't do it. Especially pr_info() ?!
Though I don't feel strongly about it.
Honestly speaking, caller should check for errors and print appropriate
messages. info->wd.mnt->mnt_root != info->wd.dentry indicates that something
went wrong (maybe memory corruption). But other conditions are not fatal.
That is, I consider even pr_info() here should be unnecessary.
They were all should never happen cases. Which is why my patches do:
if (WARN_ON_ONCE(...))
No. Fuzz testing (which uses panic_on_warn=1) will trivially hit them.
I don't believe that's true.
Please show fuzzing stack trace to prove your point.
On Tue, Jun 30, 2020 at 03:28:49PM +0900, Tetsuo Handa wrote:
quoted
On 2020/06/30 5:19, Eric W. Biederman wrote:
quoted
Tetsuo Handa [off-list ref] writes:
quoted
On 2020/06/29 4:44, Alexei Starovoitov wrote:
quoted
But all the defensive programming kinda goes against general kernel style.
I wouldn't do it. Especially pr_info() ?!
Though I don't feel strongly about it.
Honestly speaking, caller should check for errors and print appropriate
messages. info->wd.mnt->mnt_root != info->wd.dentry indicates that something
went wrong (maybe memory corruption). But other conditions are not fatal.
That is, I consider even pr_info() here should be unnecessary.
They were all should never happen cases. Which is why my patches do:
if (WARN_ON_ONCE(...))
No. Fuzz testing (which uses panic_on_warn=1) will trivially hit them.
I don't believe that's true.
Please show fuzzing stack trace to prove your point.
On Tue, Jun 30, 2020 at 2:55 PM Tetsuo Handa
[off-list ref] wrote:
On 2020/07/01 1:48, Alexei Starovoitov wrote:
quoted
On Tue, Jun 30, 2020 at 03:28:49PM +0900, Tetsuo Handa wrote:
quoted
On 2020/06/30 5:19, Eric W. Biederman wrote:
quoted
Tetsuo Handa [off-list ref] writes:
quoted
On 2020/06/29 4:44, Alexei Starovoitov wrote:
quoted
But all the defensive programming kinda goes against general kernel style.
I wouldn't do it. Especially pr_info() ?!
Though I don't feel strongly about it.
Honestly speaking, caller should check for errors and print appropriate
messages. info->wd.mnt->mnt_root != info->wd.dentry indicates that something
went wrong (maybe memory corruption). But other conditions are not fatal.
That is, I consider even pr_info() here should be unnecessary.
They were all should never happen cases. Which is why my patches do:
if (WARN_ON_ONCE(...))
No. Fuzz testing (which uses panic_on_warn=1) will trivially hit them.
I don't believe that's true.
Please show fuzzing stack trace to prove your point.
Is it a joke? Do you understand how syzbot works?
If so, please explain how it can invoke umd_* interface.
Currently syzkaller can't invoke umd_* interface because this interface is used by only
bpfilter_umh module. But I can imagine that someone starts using this interface in a way
syzkaller can somehow invoke. Thus, how can it be a joke? I don't understand your question.