Re: [statfs12] CVS commit: src
>
> 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
signature.asc
Description: Message signed with OpenPGP
Re: CVS commit: src/sys/arch/powerpc/include
Perhaps yes, but I'm not sure. How do you mips guys think?
Thanks,
rin
On 2020/06/27 19:08, Jaromír Doleček wrote:
Perhaps we can use a similar technique for mips too? There also the
kernel actually always uses a compile-time fixed page size AFAICS.
Jaromir
Le sam. 27 juin 2020 à 04:51, Rin Okuyama a écrit :
Module Name:src
Committed By: rin
Date: Sat Jun 27 02:51:23 UTC 2020
Modified Files:
src/sys/arch/powerpc/include: vmparam.h
Log Message:
Restrict {MIN,MAX}_PAGE_SIZE for MODULAR || _MODULE, which makes
non-MODULAR kernel a little bit efficient.
They are also exposed to userland for jemalloc.
To generate a diff of this commit:
cvs rdiff -u -r1.22 -r1.23 src/sys/arch/powerpc/include/vmparam.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Re: CVS commit: src/tests/lib/libc/stdlib
On Sat, Jun 27, 2020 at 10:39:08AM +, m...@netbsd.org wrote: > > Add the default TNF copyright (2005), cf. PR misc/55419. > > I don't think we can generally do this. Can you clarify if you discussed > this with the author in commit messages? Well, the committer is christos, and I doubt he cares. But as noted in the test, this 15 year old code snippet is from another person. Like with many test cases, I suppose it was originally taken/modified from a PR or a mailing list. As I noted in PR misc/55419, the problem is generic. I doubt whether it is even possible to contact all people whose code appears in src/tests. But I guess at least all NetBSD people should add the meta-data if they have code in src/tests. - Jukka
Re: CVS commit: src/tests/lib/libc/stdlib
On Sat, Jun 27, 2020 at 10:19:43AM +, Jukka Ruohonen wrote: > Module Name: src > Committed By: jruoho > Date: Sat Jun 27 10:19:43 UTC 2020 > > Modified Files: > src/tests/lib/libc/stdlib: t_mbtowc.c > > Log Message: > Add the default TNF copyright (2005), cf. PR misc/55419. > > > To generate a diff of this commit: > cvs rdiff -u -r1.1 -r1.2 src/tests/lib/libc/stdlib/t_mbtowc.c > I don't think we can generally do this. Can you clarify if you discussed this with the author in commit messages? Thanks.
Re: CVS commit: src/sys/arch/powerpc/include
Perhaps we can use a similar technique for mips too? There also the
kernel actually always uses a compile-time fixed page size AFAICS.
Jaromir
Le sam. 27 juin 2020 à 04:51, Rin Okuyama a écrit :
>
> Module Name:src
> Committed By: rin
> Date: Sat Jun 27 02:51:23 UTC 2020
>
> Modified Files:
> src/sys/arch/powerpc/include: vmparam.h
>
> Log Message:
> Restrict {MIN,MAX}_PAGE_SIZE for MODULAR || _MODULE, which makes
> non-MODULAR kernel a little bit efficient.
>
> They are also exposed to userland for jemalloc.
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.22 -r1.23 src/sys/arch/powerpc/include/vmparam.h
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
re: CVS commit: src/sys/dev/ofw
"Martin Husemann" writes: > Module Name: src > Committed By: martin > Date: Fri Jun 26 10:14:32 UTC 2020 > > Modified Files: > src/sys/dev/ofw: ofw_subr.c > > Log Message: > Remove !cold KASSERT - it does not compile on all kernels, and it is not the > right thing to test for anyway. XXX should we panic instead? Are "compatible" > strings this long happening in real devices? IMO, best to panic with a clear message about what is wrong. .mrg.
[statfs12] Re: CVS commit: src
Module Name:src
Committed By: christos
Date: Fri Oct 4 01:28:03 UTC 2019
Modified Files:
src/lib/libc/compat/sys: compat_statfs.c
src/sys/compat/common: vfs_syscalls_20.c
src/sys/compat/sys: mount.h
Log Message:
deduplicate the conversion function from statvfs -> statfs12
To generate a diff of this commit:
cvs rdiff -u -r1.8 -r1.9 src/lib/libc/compat/sys/compat_statfs.c
cvs rdiff -u -r1.43 -r1.44 src/sys/compat/common/vfs_syscalls_20.c
cvs rdiff -u -r1.10 -r1.11 src/sys/compat/sys/mount.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
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?
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.
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.
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.
As I said, please revert all of this change, it is just plain wrong and
hasn't received any testing.
Thank you,
Maxime
