linux-kernel - Re: [PATCH v8 11/12] user_events: Validate user payloads for size and null termination

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: <20220107083252.0da5237af9c5d041a3850dc6@kernel.org>
Date:   Fri, 7 Jan 2022 08:32:52 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Beau Belgrave <beaub@...ux.microsoft.com>
Cc:     rostedt@...dmis.org, linux-trace-devel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 11/12] user_events: Validate user payloads for size
 and null termination

Hi Beau,

On Mon, 3 Jan 2022 10:53:08 -0800
Beau Belgrave <beaub@...ux.microsoft.com> wrote:

[...]
> > >  typedef void (*user_event_func_t) (struct user_event *user, struct iov_iter *i,
> > > -				   void *tpdata);
> > > +				   void *tpdata, bool *faulted);
> > 
> > Why don't you just return "int" value? ;-)
> > 
> 
> There can be more than one callback attached per-probe, and in all cases
> where a return value is needed is for a faulted (or would have faulted)
> case. This allows less branches when data is being traced/logged as the
> return value does not need to be checked (nor should it short circuit
> other probes that are attached).

Would you mean overwriting the 'faulted' ? If so, you can do something like

faulted = 0;
for_each_user_event_func(user_event_func) {
	faulted |= user_event_func();
}
if (faulted)
  ...

But I think if one user_event_func() fails to access the user data,
other funcs also fail. In this case, it is faster to skip others than
repeating faults.

[...]
> > > @@ -555,19 +648,25 @@ static void user_event_ftrace(struct user_event *user, struct iov_iter *i,
> > >  		return;
> > >  
> > >  	/* Allocates and fills trace_entry, + 1 of this is data payload */
> > > -	entry = trace_event_buffer_reserve(&event_buffer, file,
> > > -					   sizeof(*entry) + i->count);
> > > +	entry = trace_event_buffer_reserve(&event_buffer, file, size);
> > >  
> > >  	if (unlikely(!entry))
> > >  		return;
> > >  
> > > -	if (unlikely(!copy_nofault(entry + 1, i->count, i))) {
> > > -		__trace_event_discard_commit(event_buffer.buffer,
> > > -					     event_buffer.event);
> > > -		return;
> > > -	}
> > > +	if (unlikely(!copy_nofault(entry + 1, i->count, i)))
> > > +		goto discard;
> > 
> > OK, this is a fault error.
> > 
> > > +
> > > +	if (!list_empty(&user->validators) &&
> > > +	    unlikely(user_event_validate(user, entry, size)))
> > > +		goto discard;
> > 
> > But this maybe an invalid parameter error.
> > 
> 
> Yes, but it has to be an invalid parameter that would have caused a
> possible fault in a worse place. In my mind, I still treat it as a fault
> case whether the user did it intentionally or not :)

OK, I got it.

Thank you,

> 
> Thanks,
> -Beau


-- 
Masami Hiramatsu <mhiramat@...nel.org>