https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=219525
--- Comment #1 from Stephen McConnell <[email protected]> --- 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. _______________________________________________ [email protected] mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-bugs To unsubscribe, send any mail to "[email protected]"
