> 
> Please revert all of this change.
> 
> First, there was a clear vulnerability in this change, which I fixed in:
> 
>       https://mail-index.netbsd.org/source-changes/2020/06/27/msg118731.html
> 
> Then, as I said in the change, there are additional problems:
> 
> 137 static __inline int
> 138 statvfs_to_statfs12_copy(const void *vs, void *vs12, size_t l)
> 139 {
> 140   struct statfs12 *s12 = STATVFSBUF_GET();
> 141   int error;
> 142
> 143   statvfs_to_statfs12(vs, s12);
> 144   error = copyout(s12, vs12, l);
> 145   STATVFSBUF_PUT(s12);
> 146
> 147   return error;
> 148 }
> 
> STATVFSBUF_GET() allocates struct statvfs, but here we're using struct
> statfs12. How can this be expected to be correct?

It is larger than needed, so it works.

> Then the copyout is done with a size, and again there are problems here.
> In compat_20_sys_getfsstat() the size given is struct statvfs90, which
> matches neither the filled size nor the allocated size.

That is a mistake and I have fixed it.

> The other callers have even bigger problems. For example
> compat_20_sys_statfs() passes zero as size. So the result simply never
> gets copied out. How can this be expected to be correct? As far as I can
> tell the syscall simply cannot work now.

Same bug as above.

> Finally, I don't even understand what this change dedups. It just moved
> the functions from one place to another, introduced bugs in them, but
> didn't reduce the code size in any way.

It reduces maintainability, since the conversion was done in two places
(in libc and the kernel) and now it is done in one.

> As I said, please revert all of this change, it is just plain wrong and
> hasn't received any testing.

I have fixed it instead, if you find more bugs please let me know.

christos

Attachment: signature.asc
Description: Message signed with OpenPGP

Reply via email to