On Tue, Apr 13, 2021 at 09:36:32AM +0200, Peter Zijlstra wrote:
> On Mon, Apr 12, 2021 at 01:05:09PM -0700, Kees Cook wrote:
> > On Mon, Apr 12, 2021 at 10:00:16AM +0200, Peter Zijlstra wrote:
> > > +struct vpr_data {
> > > + int (*fn)(pte_t pte, unsigned long addr, void *data);
> > > + void *data;
> > > +};
> >
> > Eeerg. This is likely to become an attack target itself. Stored function
> > pointer with stored (3rd) argument.
> >
> > This doesn't seem needed: only DRM uses it, and that's for error
> > reporting. I'd rather plumb back errors in a way to not have to add
> > another place in the kernel where we do func+arg stored calling.
>
> Is this any better? It does have the stored pointer, but not a stored
> argument, assuming you don't count returns as arguments I suppose.
It's better in the sense that it's not the func/arg pair that really
bugs me, yes. :)
> The alternative is refactoring apply_to_page_range() :-/
Yeah, I'm looking now, I see what you mean.
> ---
>
> struct vpr_data {
> bool (*fn)(pte_t pte, unsigned long addr);
> unsigned long addr;
> };
>
> static int vpr_fn(pte_t *pte, unsigned long addr, void *data)
> {
> struct vpr_data *vpr = data;
> if (!vpr->fn(*pte, addr)) {
> vpr->addr = addr;
> return -EINVAL;
> }
> return 0;
> }
My point about passing "addr" was that nothing in the callback actually
needs it -- the top level can just as easily report the error. And that
the helper is always vpr_fn(), so it doesn't need to be passed either.
So the addr can just be encoded in "int", and no structure is needed at:
typedef bool (*vpr_fn_t)(pte_t pte);
static int vpr_fn(pte_t *pte, unsigned long addr, void *data)
{
vpr_fn_t callback = data;
if (!callback(*pte))
return addr >> PAGE_SIZE;
return 0;
}
unsigned long verify_page_range(struct mm_struct *mm,
unsigned long addr, unsigned long size,
vpr_fn_t callback)
{
return apply_to_page_range(mm, addr, size, vpr_fn, callback) << PAGE_SIZE;
}
But maybe I'm missing something?
--
Kees Cook
