|
From: Duncan S. <bal...@fr...> - 2006-03-01 11:36:42
|
Consider a system call like mmap. The fd
parameter is not accessed if MAP_ANONYMOUS
is set in flags, so it doesn't matter if it
contains junk. However valgrind complains
that
"Syscall param mmap2(fd) contains uninitialised byte(s)"
This seems to be due to the following
PRE_REG_READ6(long, "mmap2",
unsigned long, start, unsigned long, length,
unsigned long, prot, unsigned long, flags,
unsigned long, fd, unsigned long, offset);
at line 1303 of syswrap-x86-linux.c, which appears to check
that none of the arguments contains junk.
Is this a valgrind bug, or is there a policy of checking all
syscall arguments regardless of whether they will be used or
not?
Thanks for clarifying.
All the best,
Duncan.
|
|
From: Julian S. <js...@ac...> - 2006-03-01 12:18:34
|
> Is this a valgrind bug, or is there a policy of checking all > syscall arguments regardless of whether they will be used or > not? I think it's a bug. Or not exactly a bug: V's syscall wrappers constitute an approximate model of how the kernel uses and modifies user-space memory across a syscall. What you've found is a place where the model is insufficiently accurate. Do you have a patch to improve it? J |
|
From: Duncan S. <bal...@fr...> - 2006-03-01 21:42:31
|
Hi Julian,
> > Is this a valgrind bug, or is there a policy of checking all
> > syscall arguments regardless of whether they will be used or
> > not?
>
> I think it's a bug. Or not exactly a bug: V's syscall wrappers constitute
> an approximate model of how the kernel uses and modifies user-space
> memory across a syscall. What you've found is a place where the model is
> insufficiently accurate. Do you have a patch to improve it?
thanks for replying. I'll be happy to produce a patch, but first I have
some questions. This call tells valgrind that all arguments are being
read, right?
PRE_REG_READ6(long, "mmap2",
unsigned long, start, unsigned long, length,
unsigned long, prot, unsigned long, flags,
unsigned long, fd, unsigned long, offset);
So presumably I should do PRE_REG_READ4, a PRRAn on the offset
argument, and a PRRAn of the fd argument depending on the value
of flags. But perhaps there is a better way? I notice that
PPRAn is not used anywhere outside of priv_types_n_macros.h,
which suggests that this is the wrong approach.
All the best,
Duncan.
|
|
From: Julian S. <js...@ac...> - 2006-03-01 23:33:20
|
Hi Duncan Modulo my other reply to list on this topic (fix clone, don't fix mmap) I think your understanding is correct: > some questions. This call tells valgrind that all arguments are being > read, right? > > PRE_REG_READ6(long, "mmap2", > unsigned long, start, unsigned long, length, > unsigned long, prot, unsigned long, flags, > unsigned long, fd, unsigned long, offset); I believe so. (peering at priv_types_n_macros.h seems to confirm that). > So presumably I should do PRE_REG_READ4, a PRRAn on the offset > argument, and a PRRAn of the fd argument depending on the value > of flags. That sounds reasonable. Just make sure you do a definedness check on any value you "if" on in the wrapper. But in this case you'd be looking at "flags", which would be tested by a PRE_REG_READ4, so you're ok. > But perhaps there is a better way? Not that I know of. > I notice that > PPRAn is not used anywhere outside of priv_types_n_macros.h, > which suggests that this is the wrong approach. Hmm. Not sure. Ah - one thing. Observe that the PRE_REG_READ macros all start off "if (VG_(tdict).track_pre_reg_read) ...". This stops them using VG_(tdict).track_pre_reg_read in cases where the tool does not care about tracking shadow register values. This means you need to guard all uses of PRRAn with that test, otherwise the system will surely segfault for most tools /= memcheck. Hmm. That's ugly. Maybe there is a better way to conditionally do definedness checks of some scalar syscall args depending on the value of other args. Nick and/or Tom should know, if that is the case. J |
|
From: Tom H. <to...@co...> - 2006-03-01 12:24:24
|
In message <200...@fr...>
Duncan Sands <bal...@fr...> wrote:
> Consider a system call like mmap. The fd
> parameter is not accessed if MAP_ANONYMOUS
> is set in flags, so it doesn't matter if it
> contains junk. However valgrind complains
> that
> "Syscall param mmap2(fd) contains uninitialised byte(s)"
> This seems to be due to the following
>
> PRE_REG_READ6(long, "mmap2",
> unsigned long, start, unsigned long, length,
> unsigned long, prot, unsigned long, flags,
> unsigned long, fd, unsigned long, offset);
>
> at line 1303 of syswrap-x86-linux.c, which appears to check
> that none of the arguments contains junk.
>
> Is this a valgrind bug, or is there a policy of checking all
> syscall arguments regardless of whether they will be used or
> not?
Obviously it is a bit of a bug, but is not trivial to fix and
rarely causes a problem as people normal pass a constant -1 or
something when doing an anonymous map.
Tom
--
Tom Hughes (to...@co...)
http://www.compton.nu/
|
|
From: Duncan S. <bal...@fr...> - 2006-03-01 21:44:24
|
Hi Tom, > Obviously it is a bit of a bug, but is not trivial to fix and > rarely causes a problem as people normal pass a constant -1 or > something when doing an anonymous map. I agree that it is not very important (and in fact I myself am not much interested in it - I'm interested in a more complicated case with sys_clone; mmap is a warmup). But why do you say it is not trivial to fix? Thanks, Duncan. |
|
From: Tom H. <to...@co...> - 2006-03-02 19:22:50
|
In message <200...@fr...>
Duncan Sands <bal...@fr...> wrote:
> > Obviously it is a bit of a bug, but is not trivial to fix and
> > rarely causes a problem as people normal pass a constant -1 or
> > something when doing an anonymous map.
>
> I agree that it is not very important (and in fact I myself am
> not much interested in it - I'm interested in a more complicated
> case with sys_clone; mmap is a warmup). But why do you say it is
> not trivial to fix?
I was really just referring to the fact that checking the registers
is bound up in those macros that check everything at once, but it
looks like you have all that worked out.
Tom
--
Tom Hughes (to...@co...)
http://www.compton.nu/
|
> Consider a system call like mmap. The fd > parameter is not accessed if MAP_ANONYMOUS > is set in flags, so it doesn't matter if it > contains junk. However valgrind complains > that > "Syscall param mmap2(fd) contains uninitialised byte(s)" I'd say that Valgrind has done you a favor. The message is _exactly_ correct: the program passed uninitialized bytes into the operating system kernel. It happens that today under proper operation these bytes are "don't care": the kernel ignores them. However, from the viewpoint of information security, the program has "leaked" information to a place where there is no need for it to go. Those uninitialized bytes do have a value that was picked up from some discarded state in the program. Perhaps the value is one that would better be kept "secret." Threats posed by malware of various kinds also suggest that it is a good idea not to pass around "uninitialized" bytes. If there should be a program bug somewhere, then uninitialized bytes have a knack for acting as catalyst to amplify the damage and/or spread the confusion and diffuse the recognizable error states. Someday, knowingly processing uninitialized bytes may become a legal liability. [Yeah, that last part is FUD. But the probability of it happening is not zero.] -- |
|
From: Nicholas N. <nj...@cs...> - 2006-03-05 22:39:08
|
On Wed, 1 Mar 2006, Duncan Sands wrote:
> Consider a system call like mmap. The fd
> parameter is not accessed if MAP_ANONYMOUS
> is set in flags, so it doesn't matter if it
> contains junk. However valgrind complains
> that
> "Syscall param mmap2(fd) contains uninitialised byte(s)"
> This seems to be due to the following
>
> PRE_REG_READ6(long, "mmap2",
> unsigned long, start, unsigned long, length,
> unsigned long, prot, unsigned long, flags,
> unsigned long, fd, unsigned long, offset);
>
> at line 1303 of syswrap-x86-linux.c, which appears to check
> that none of the arguments contains junk.
>
> Is this a valgrind bug, or is there a policy of checking all
> syscall arguments regardless of whether they will be used or
> not?
Here's the code for open(), which only sometimes looks at the 3rd argument:
if (ARG2 & VKI_O_CREAT) {
// 3-arg version
PRINT("sys_open ( %p(%s), %d, %d )",ARG1,ARG1,ARG2,ARG3);
PRE_REG_READ3(long, "open",
const char *, filename, int, flags, int, mode);
} else {
// 2-arg version
PRINT("sys_open ( %p(%s), %d )",ARG1,ARG1,ARG2);
PRE_REG_READ2(long, "open",
const char *, filename, int, flags);
}
So I think the current policy is to only check the arguments that are used
by the kernel, and that the mmap() wrapper was not implementing that policy
correctly. I also appreciate John's point about leaking information into
the kernel.
Nick
|
|
From: Duncan S. <bal...@fr...> - 2006-03-06 09:36:38
|
Hi Nick,
> ...
> > Is this a valgrind bug, or is there a policy of checking all
> > syscall arguments regardless of whether they will be used or
> > not?
>
> Here's the code for open(), which only sometimes looks at the 3rd argument:
>
> if (ARG2 & VKI_O_CREAT) {
> // 3-arg version
> PRINT("sys_open ( %p(%s), %d, %d )",ARG1,ARG1,ARG2,ARG3);
> PRE_REG_READ3(long, "open",
> const char *, filename, int, flags, int, mode);
> } else {
> // 2-arg version
> PRINT("sys_open ( %p(%s), %d )",ARG1,ARG1,ARG2);
> PRE_REG_READ2(long, "open",
> const char *, filename, int, flags);
> }
isn't this code a bit naughty, since the call to ARG2 reads a potentially
uninitialised value, before any call to PRE_REG_READx?
> So I think the current policy is to only check the arguments that are used
> by the kernel, and that the mmap() wrapper was not implementing that policy
> correctly. I also appreciate John's point about leaking information into
> the kernel.
I don't really see a problem with leaking information into the kernel. It
seems like a particularly safe place to send stuff!
All the best,
Duncan.
|
|
From: Julian S. <js...@ac...> - 2006-03-06 10:43:06
|
> > Here's the code for open(), which only sometimes looks at the 3rd
> > argument:
> >
> > if (ARG2 & VKI_O_CREAT) {
> > // 3-arg version
> > PRINT("sys_open ( %p(%s), %d, %d )",ARG1,ARG1,ARG2,ARG3);
> > PRE_REG_READ3(long, "open",
> > const char *, filename, int, flags, int, mode);
> > } else {
> > // 2-arg version
> > PRINT("sys_open ( %p(%s), %d )",ARG1,ARG1,ARG2);
> > PRE_REG_READ2(long, "open",
> > const char *, filename, int, flags);
> > }
>
> isn't this code a bit naughty, since the call to ARG2 reads a potentially
> uninitialised value, before any call to PRE_REG_READx?
It is kind-of, but it does (I think) check ARG2 on both paths, so the
error will be detected eventually, although perhaps on the wrong path.
Any progress on improving the clone wrappers?
J
|
|
From: Julian S. <js...@ac...> - 2006-03-06 14:41:29
|
> [arg checking for sys_clone] Duncan - I've just realised #117564 is exactly the problem you're talking about. See https://bugs.kde.org/show_bug.cgi?id=117564 You might want to look at Jeroen's patch, which is attached to said web page. J |
Duncan Sands wrote: > Hi Nick, >>So I think the current policy is to only check the arguments that are used >>by the kernel, and that the mmap() wrapper was not implementing that policy >>correctly. I also appreciate John's point about leaking information into >>the kernel. > > > I don't really see a problem with leaking information into the kernel. It > seems like a particularly safe place to send stuff! This is a fallacy. The kernel is a particularly *dangerous* place. First of all, the kernel has deliberate "observation points" *designed* to expose what is going on: ptrace, strace, /proc/<pid>, /bin/ps, /bin/top, etc. Second, the Linux kernel is somewhat blase about all its privileges: it has no internal protection domains, "firewalls", etc. Yes, in theory the kernel can do anything to destroy information security; but there is no need for user-mode code to tempt the kernel and/or make it easy for such lapses. -- |
|
From: Duncan S. <bal...@fr...> - 2006-03-06 16:01:37
|
> > I don't really see a problem with leaking information into the kernel. It > > seems like a particularly safe place to send stuff! > > This is a fallacy. The kernel is a particularly *dangerous* place. > > First of all, the kernel has deliberate "observation points" *designed* > to expose what is going on: ptrace, strace, /proc/<pid>, /bin/ps, > /bin/top, etc. Yes, but if you have sufficient privileges to use them against a program, you already have sufficient privileges to eg attach a debugger to it and rummage around directly. > Second, the Linux kernel is somewhat blase about all its > privileges: it has no internal protection domains, "firewalls", etc. Are you worried about local attackers, or rather that your uninitialised bytes might accidentally be sent off in an ethernet packet or something like that? > Yes, in theory the kernel can do anything to destroy information security; > but there is no need for user-mode code to tempt the kernel and/or > make it easy for such lapses. s/easy/easier/ Although it may not appear that way, I have a plenty of sympathy for your "code hygiene" argument. Ciao, Duncan. |
|
From: Duncan S. <bal...@fr...> - 2006-03-06 14:51:59
|
> > [arg checking for sys_clone] > > Duncan - I've just realised #117564 is exactly the problem you're > talking about. See https://bugs.kde.org/show_bug.cgi?id=117564 > You might want to look at Jeroen's patch, which is attached to > said web page. I see he chose to use PRRAn; I was planning to introduce a more descriptive name. Besides that, it looks good at first glance. Ciao, D. PS: I hope to find time to do something soon. |
|
From: Tom H. <to...@co...> - 2006-03-06 15:07:56
|
In message <200...@fr...>
Duncan Sands <bal...@fr...> wrote:
>> > [arg checking for sys_clone]
>>
>> Duncan - I've just realised #117564 is exactly the problem you're
>> talking about. See https://bugs.kde.org/show_bug.cgi?id=117564
>> You might want to look at Jeroen's patch, which is attached to
>> said web page.
>
> I see he chose to use PRRAn; I was planning to introduce a more
> descriptive name. Besides that, it looks good at first glance.
If you are playing with the clone argument checking, be very
careful about cutting and pasting from one platform to another
as the arguments are in different orders on different platforms...
Tom
--
Tom Hughes (to...@co...)
http://www.compton.nu/
|
|
From: Julian S. <js...@ac...> - 2006-03-06 15:23:09
|
On Monday 06 March 2006 15:07, Tom Hughes wrote: > In message <200...@fr...> > > Duncan Sands <bal...@fr...> wrote: > >> > [arg checking for sys_clone] > >> > >> Duncan - I've just realised #117564 is exactly the problem you're > >> talking about. See https://bugs.kde.org/show_bug.cgi?id=117564 > >> You might want to look at Jeroen's patch, which is attached to > >> said web page. > > > > I see he chose to use PRRAn; I was planning to introduce a more > > descriptive name. Besides that, it looks good at first glance. Except PRRAn is used without a guarding check of if (VG_(tdict).track_pre_reg_read). Maybe you could invent a new macro with a better name and which incorporates that check. > If you are playing with the clone argument checking, be very > careful about cutting and pasting from one platform to another > as the arguments are in different orders on different platforms... Yes - I remember you fixed some swamp-age to do with this. > PS: I hope to find time to do something soon. Am hoping to finalise 3.1.1 in the next day or so, so as to ship it around Friday. If you have something by then, fine, else we can ship a fix in 3.2.0 (mid-late April). J |
|
From: Duncan S. <bal...@fr...> - 2006-03-07 11:00:11
|
Hi Tom,
> > I see he chose to use PRRAn; I was planning to introduce a more
> > descriptive name. Besides that, it looks good at first glance.
>
> If you are playing with the clone argument checking, be very
> careful about cutting and pasting from one platform to another
> as the arguments are in different orders on different platforms...
indeed. By the way, only x86 seems to have this bit of logic:
if (ARG1 & VKI_CLONE_SETTLS) {
PRE_MEM_READ("clone(tls_user_desc)", ARG4, sizeof(vki_modify_ldt_t));
if (!VG_(am_is_valid_for_client)(ARG4, sizeof(vki_modify_ldt_t),
VKI_PROT_READ)) {
SET_STATUS_Failure( VKI_EFAULT );
return;
}
}
Do you know why - is newtls unused on other platforms? (I will have
a look at the kernel source to find out, but I thought you might know).
Ciao,
Duncan.
|
|
From: Tom H. <to...@co...> - 2006-03-07 11:10:43
|
In message <200...@fr...>
Duncan Sands <bal...@fr...> wrote:
> indeed. By the way, only x86 seems to have this bit of logic:
>
> if (ARG1 & VKI_CLONE_SETTLS) {
> PRE_MEM_READ("clone(tls_user_desc)", ARG4, sizeof(vki_modify_ldt_t));
> if (!VG_(am_is_valid_for_client)(ARG4, sizeof(vki_modify_ldt_t),
> VKI_PROT_READ)) {
> SET_STATUS_Failure( VKI_EFAULT );
> return;
> }
> }
>
> Do you know why - is newtls unused on other platforms? (I will have
> a look at the kernel source to find out, but I thought you might know).
Possibly - the way TLS data works is certainly different on amd64. No
idea how the PPC platforms handle it.
Tom
--
Tom Hughes (to...@co...)
http://www.compton.nu/
|
|
From: Paul M. <pa...@sa...> - 2006-03-07 12:03:59
|
Tom Hughes writes:
> In message <200...@fr...>
> Duncan Sands <bal...@fr...> wrote:
>
> > indeed. By the way, only x86 seems to have this bit of logic:
> >
> > if (ARG1 & VKI_CLONE_SETTLS) {
> > PRE_MEM_READ("clone(tls_user_desc)", ARG4, sizeof(vki_modify_ldt_t));
> > if (!VG_(am_is_valid_for_client)(ARG4, sizeof(vki_modify_ldt_t),
> > VKI_PROT_READ)) {
> > SET_STATUS_Failure( VKI_EFAULT );
> > return;
> > }
> > }
> >
> > Do you know why - is newtls unused on other platforms? (I will have
> > a look at the kernel source to find out, but I thought you might know).
>
> Possibly - the way TLS data works is certainly different on amd64. No
> idea how the PPC platforms handle it.
If I recall correctly, the CLONE_SETTLS flag just causes the kernel to
set the thread pointer register (r2 for 32-bit, r13 for 64-bit) for
the new task to the tls argument. That is, the tls argument doesn't
point to a structure in memory, it's just a value. (Probably Valgrind
should transfer the definedness of the argument to the target r2/r13.)
Paul.
|
|
From: Duncan S. <bal...@fr...> - 2006-03-08 15:15:21
|
On Tuesday 7 March 2006 13:03, Paul Mackerras wrote:
> Tom Hughes writes:
>
> > In message <200...@fr...>
> > Duncan Sands <bal...@fr...> wrote:
> >
> > > indeed. By the way, only x86 seems to have this bit of logic:
> > >
> > > if (ARG1 & VKI_CLONE_SETTLS) {
> > > PRE_MEM_READ("clone(tls_user_desc)", ARG4, sizeof(vki_modify_ldt_t));
> > > if (!VG_(am_is_valid_for_client)(ARG4, sizeof(vki_modify_ldt_t),
> > > VKI_PROT_READ)) {
> > > SET_STATUS_Failure( VKI_EFAULT );
> > > return;
> > > }
> > > }
> > >
> > > Do you know why - is newtls unused on other platforms? (I will have
> > > a look at the kernel source to find out, but I thought you might know).
> >
> > Possibly - the way TLS data works is certainly different on amd64. No
> > idea how the PPC platforms handle it.
>
> If I recall correctly, the CLONE_SETTLS flag just causes the kernel to
> set the thread pointer register (r2 for 32-bit, r13 for 64-bit) for
> the new task to the tls argument. That is, the tls argument doesn't
> point to a structure in memory, it's just a value. (Probably Valgrind
> should transfer the definedness of the argument to the target r2/r13.)
Sorry for the dumb question, but should I be looking in arch/ppc or
arch/powerpc?
Thanks,
Duncan.
|
|
From: Julian S. <js...@ac...> - 2006-03-08 15:24:30
|
> Sorry for the dumb question, but should I be looking in arch/ppc or > arch/powerpc? Not sure what you mean here - the V tree has no suchnamed directories. Are you referring the the glibc tree? J |
|
From: Tom H. <to...@co...> - 2006-03-08 15:41:28
|
In message <200...@ac...>
Julian Seward <js...@ac...> wrote:
>> Sorry for the dumb question, but should I be looking in arch/ppc or
>> arch/powerpc?
>
> Not sure what you mean here - the V tree has no suchnamed directories.
> Are you referring the the glibc tree?
I think he means the kernel tree.
Tom
--
Tom Hughes (to...@co...)
http://www.compton.nu/
|
|
From: Duncan S. <bal...@fr...> - 2006-03-08 15:44:42
|
On Wednesday 8 March 2006 16:21, Julian Seward wrote: > > > Sorry for the dumb question, but should I be looking in arch/ppc or > > arch/powerpc? > > Not sure what you mean here - the V tree has no suchnamed directories. > Are you referring the the glibc tree? The kernel tree. Sorry for the confusion, Duncan. |
|
From: Paul M. <pa...@sa...> - 2006-03-09 00:35:39
|
Duncan Sands writes: > > If I recall correctly, the CLONE_SETTLS flag just causes the kernel to > > set the thread pointer register (r2 for 32-bit, r13 for 64-bit) for > > the new task to the tls argument. That is, the tls argument doesn't > > point to a structure in memory, it's just a value. (Probably Valgrind > > should transfer the definedness of the argument to the target r2/r13.) > > Sorry for the dumb question, but should I be looking in arch/ppc or > arch/powerpc? arch/powerpc. Paul. |