[Bug 219525] Multiple bugs in mpr ioctl handler

[bugzilla-noreply at freebsd.org]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=219525

--- Comment #1 from Stephen McConnell <slm at freebsd.org> ---
Thanks for the report. I know there are a few issues there, especially with the
driver not checking for bad user supplied values. I agree with most of your
comments, but I have a question.

In this section:
.....
        mpr_lock(sc);
        size = data->Size;
        if ((size >= sizeof(sc->recorded_events)) && (status == 0)) {
            mpr_unlock(sc);
            if (copyout((void *)sc->recorded_events,
                PTRIN(data->PtrEvents), size) != 0)
                status = EFAULT;
            mpr_lock(sc);
        } else {
            /*
             * data->Size value is not large enough to copy event data.
             */
            status = EFAULT;
        }

        /*
         * Change size value to match the number of bytes that were copied.
         */
        if (status == 0)
            data->Size = sizeof(sc->recorded_events);
        mpr_unlock(sc);
.....

You say:
"It checks if the user supplied size is greater than the the size of the
struct, and if so, copies using the size which it just determined was too
large. It should `copyout` using `sizeof(sc->recorded_events)`."

But this is just checking if the user supplied size is enough to hold
sizeof(sc->recored_events). It's not checking if it was too large, it's
checking if it's large enough. If that user supplied size is >= to the data
being copied, it should be OK. Do you agree?

I'm not sure I like that data->Size is being changed from the user supplied
value, but that's something different. Maybe that's OK, but not sure. It could
be that this is a normal technique used to return the number of bytes that are
valid in the user buffer, but I only see that being done in this one function.

-- 
You are receiving this mail because:
You are the assignee for the bug.