Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
On Wed, Aug 28, 2013 at 09:43:24PM +0200, Maxime Villard wrote:
> On 08/28/13 20:57, Matthew Dempsky wrote:
> > On Wed, Aug 28, 2013 at 5:54 AM, Maxime Villard wrote:
> >> + /* Ensure interp is a valid, NUL-terminated string
> >> */
> >> + for (n = 0; n < pp->p_filesz; n++) {
> >> + if (interp[n] == '\0')
> >> + break;
> >> + }
> >> + if (n != pp->p_filesz - 1) {
> >> + error = ENOEXEC;
> >> goto bad;
> >> }
> >
> > A more concise test would be:
> >
> > if (strnlen(interp, pp->p_filesz) != pp->p_filesz - 1) {
> > error = ENOEXEC;
> > goto bad;
> > }
> >
>
> Hum you're right, it's better that way
>
>
> Index: exec_elf.c
> ===
> RCS file: /cvs/src/sys/kern/exec_elf.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 exec_elf.c
> --- exec_elf.c4 Jul 2013 17:37:05 - 1.93
> +++ exec_elf.c28 Aug 2013 21:14:22 -
> @@ -552,11 +552,16 @@ ELFNAME2(exec,makecmds)(struct proc *p,
>
> for (i = 0, pp = ph; i < eh->e_phnum; i++, pp++) {
> if (pp->p_type == PT_INTERP && !interp) {
> - if (pp->p_filesz >= MAXPATHLEN)
> + if (pp->p_filesz < 2 || pp->p_filesz >= MAXPATHLEN)
Still think you're depriving yourself of one character out by using
">=" instead of ">".
Ken
> goto bad;
> interp = pool_get(&namei_pool, PR_WAITOK);
> if ((error = ELFNAME(read_from)(p, epp->ep_vp,
> pp->p_offset, interp, pp->p_filesz)) != 0) {
> + goto bad;
> + }
> + /* Ensure interp is NUL-terminated and of the expected
> length */
> + if (strnlen(interp, pp->p_filesz) != pp->p_filesz - 1) {
> + error = ENOEXEC;
> goto bad;
> }
> } else if (pp->p_type == PT_LOAD) {
>
>
> It no longer looks like my patch...
>
Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
On Wed, Aug 28, 2013 at 08:44:26PM +0200, Maxime Villard wrote:
> On 08/28/13 16:30, Kenneth R Westerback wrote:
> > On Wed, Aug 28, 2013 at 02:54:11PM +0200, Maxime Villard wrote:
> >> Updated diff, with small tweaks from Andres Perera,
> >> * int -> size_t, signedness issue, even if it can't be >INT_MAX
> >> * NULL -> NUL
> >>
> >>
> >> Index: exec_elf.c
> >> ===
> >> RCS file: /cvs/src/sys/kern/exec_elf.c,v
> >> retrieving revision 1.93
> >> diff -u -p -r1.93 exec_elf.c
> >> --- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
> >> +++ exec_elf.c 28 Aug 2013 14:36:58 -
> >> @@ -516,7 +516,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
> >>int error, i;
> >>char *interp = NULL;
> >>u_long pos = 0, phsize;
> >> - size_t randomizequota = ELF_RANDOMIZE_LIMIT;
> >> + size_t n, randomizequota = ELF_RANDOMIZE_LIMIT;
> >>
> >>if (epp->ep_hdrvalid < sizeof(Elf_Ehdr))
> >>return (ENOEXEC);
> >> @@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
> >>
> >>for (i = 0, pp = ph; i < eh->e_phnum; i++, pp++) {
> >>if (pp->p_type == PT_INTERP && !interp) {
> >> - if (pp->p_filesz >= MAXPATHLEN)
> >> + if (pp->p_filesz >= MAXPATHLEN ||
> >> + pp->p_filesz < 2)
> >
> > Since p_filesz includes the NUL and MAXPATHLEN includes the NUL as
> > far as I know, could the comparison not be simply '> MAXPATHLEN'?
> >
> > In passing I note that 37 out of 749 declarations using MAXPATHLEN in the
> > tree use 'char blah[MAXPATHLEN + 1]'. No idea if it is worth looking at
> > making them do without the '+ 1'. :-)
> >
> > I also note in passing several "#define PATH_MAX (MAXPATHLEN + 1)'
> > declarations, which strike me as odd since MAXPATHLEN is defined
> > in sys/param.h by '#define MAXPATHEN PATH_MAX'.
> >
> > Let the POSIX lawyers apply any necessary cluebats please.
> >
> >>goto bad;
> >>interp = pool_get(&namei_pool, PR_WAITOK);
> >>if ((error = ELFNAME(read_from)(p, epp->ep_vp,
> >>pp->p_offset, interp, pp->p_filesz)) != 0) {
> >> + goto bad;
> >> + }
> >> + /* Ensure interp is a valid, NUL-terminated string */
> >
> > The condition is not that the string is NUL terminated ("aaa\0aaa\0" is
> > after
> > all NUL terminated and valid as far as any function reading strings would be
> > concerned. It just has trailing garbage.) Perhaps the comment should refer
> > to making sure the string is spec compliant by being of the expected length.
>
>
> By 'valid string' I actually meant 'without trailing garbage'... but ok
> if it's not clear
> /* Ensure interp is NUL-terminated and of the expected length */
That would be clear to me.
Ken
>
>
> >
> > Ken
> >
> >> + for (n = 0; n < pp->p_filesz; n++) {
> >> + if (interp[n] == '\0')
> >> + break;
> >> + }
> >> + if (n != pp->p_filesz - 1) {
> >> + error = ENOEXEC;
> >>goto bad;
> >>}
> >>} else if (pp->p_type == PT_LOAD) {
> >>
> >>
> >>
> >> On 08/28/13 08:44, Maxime Villard wrote:
> >>> Hi,
> >>> in the ELF format, the PT_INTERP segment contains the path of the
> >>> interpreter which must be loaded. For this segment, the kernel looks
> >>> at these values in the program header:
> >>>
> >>>p_offset: offset of the path string
> >>>p_filesz: size of the path string, including the \0
> >>>
> >>> The path string must be a valid string, or the binary won't be loaded.
> >>>
> >>> The kernel actually doesn't check the string, it just reads p_filesz
> >>> bytes from p_offset, and later it checks if it can be loaded. Malformed
> >>> binaries which have paths like:
> >>>
> >>> "a"
> >>> or
> >>> "aaa\0aaa\0aaa\0aaa\0"
> >>>
> >>> are parsed, loaded, and then the kernel figures out that the path is
> >>> not valid, and aborts.
> >>>
> >>> When the kernel reads the path from the file, it should check directly
> >>> the string to ensure it is at least a valid string.
> >>>
> >>> Here is a patch for this. For pp->p_filesz, I could have put
> >>> if (pp->p_filesz == 0)
> >>> but values of 1 also don't make sense; "\0" can't be a valid path.
> >>>
> >>>
> >>>
> >>> Index: exec_elf.c
> >>> ===
> >>> RCS file: /cvs/src/sys/kern/exec_elf.c,v
> >>> retrieving revision 1.93
> >>> diff -u -p -r1.93 exec_elf.c
> >>> --- exec_elf.c4 Jul 2013 17:37:05 - 1.93
> >>> +++ exec_elf.c27 Aug 2013 22:59:21 -
> >>> @@ -513,7 +513,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
> >>> Elf_Ehdr *eh = epp->ep_hdr;
> >>> Elf_Phdr *ph
Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
On 08/28/13 20:57, Matthew Dempsky wrote:
> On Wed, Aug 28, 2013 at 5:54 AM, Maxime Villard wrote:
>> + /* Ensure interp is a valid, NUL-terminated string */
>> + for (n = 0; n < pp->p_filesz; n++) {
>> + if (interp[n] == '\0')
>> + break;
>> + }
>> + if (n != pp->p_filesz - 1) {
>> + error = ENOEXEC;
>> goto bad;
>> }
>
> A more concise test would be:
>
> if (strnlen(interp, pp->p_filesz) != pp->p_filesz - 1) {
> error = ENOEXEC;
> goto bad;
> }
>
Hum you're right, it's better that way
Index: exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -p -r1.93 exec_elf.c
--- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
+++ exec_elf.c 28 Aug 2013 21:14:22 -
@@ -552,11 +552,16 @@ ELFNAME2(exec,makecmds)(struct proc *p,
for (i = 0, pp = ph; i < eh->e_phnum; i++, pp++) {
if (pp->p_type == PT_INTERP && !interp) {
- if (pp->p_filesz >= MAXPATHLEN)
+ if (pp->p_filesz < 2 || pp->p_filesz >= MAXPATHLEN)
goto bad;
interp = pool_get(&namei_pool, PR_WAITOK);
if ((error = ELFNAME(read_from)(p, epp->ep_vp,
pp->p_offset, interp, pp->p_filesz)) != 0) {
+ goto bad;
+ }
+ /* Ensure interp is NUL-terminated and of the expected
length */
+ if (strnlen(interp, pp->p_filesz) != pp->p_filesz - 1) {
+ error = ENOEXEC;
goto bad;
}
} else if (pp->p_type == PT_LOAD) {
It no longer looks like my patch...
Re: Don't iterate on the global list in arp_{request,input}
On Wed, Aug 28, 2013 at 03:28:18PM +0200, Martin Pieuchot wrote:
> Like the previous diffs, when we already have the ifp and want one of
> its addresses, iterate on the ifp list instead of the global one.
>
> Tested with carp here. I appreciate any comment and oks.
I think the code is nicer and does the same. OK bluhm@
>
> Index: netinet/if_ether.c
> ===
> RCS file: /cvs/src/sys/netinet/if_ether.c,v
> retrieving revision 1.105
> diff -u -p -r1.105 if_ether.c
> --- netinet/if_ether.c28 Aug 2013 06:58:57 - 1.105
> +++ netinet/if_ether.c28 Aug 2013 07:57:01 -
> @@ -145,7 +145,6 @@ arp_rtrequest(int req, struct rtentry *r
> {
> struct sockaddr *gate = rt->rt_gateway;
> struct llinfo_arp *la = (struct llinfo_arp *)rt->rt_llinfo;
> - struct in_ifaddr *ia;
> struct ifaddr *ifa;
> struct mbuf *m;
>
> @@ -259,13 +258,13 @@ arp_rtrequest(int req, struct rtentry *r
> rt->rt_flags |= RTF_LLINFO;
> LIST_INSERT_HEAD(&llinfo_arp, la, la_list);
>
> - TAILQ_FOREACH(ia, &in_ifaddr, ia_list) {
> - if (ia->ia_ifp == rt->rt_ifp &&
> - SIN(rt_key(rt))->sin_addr.s_addr ==
> - (IA_SIN(ia))->sin_addr.s_addr)
> + TAILQ_FOREACH(ifa, &rt->rt_ifp->if_addrlist, ifa_list) {
> + if ((ifa->ifa_addr->sa_family == AF_INET) &&
> + ifatoia(ifa)->ia_addr.sin_addr.s_addr ==
> + satosin(rt_key(rt))->sin_addr.s_addr)
> break;
> }
> - if (ia) {
> + if (ifa) {
> /*
>* This test used to be
>* if (lo0ifp->if_flags & IFF_UP)
> @@ -294,7 +293,6 @@ arp_rtrequest(int req, struct rtentry *r
>* address we are using, otherwise we will have trouble
>* with source address selection.
>*/
> - ifa = &ia->ia_ifa;
> if (ifa != rt->rt_ifa) {
> ifafree(rt->rt_ifa);
> ifa->ifa_refcnt++;
> @@ -562,11 +560,12 @@ void
> in_arpinput(struct mbuf *m)
> {
> struct ether_arp *ea;
> - struct arpcom *ac = (struct arpcom *)m->m_pkthdr.rcvif;
> + struct ifnet *ifp = m->m_pkthdr.rcvif;
> + struct arpcom *ac = (struct arpcom *)ifp;
> struct ether_header *eh;
> struct llinfo_arp *la = 0;
> struct rtentry *rt;
> - struct in_ifaddr *ia;
> + struct ifaddr *ifa;
> struct sockaddr_dl *sdl;
> struct sockaddr sa;
> struct in_addr isaddr, itaddr, myaddr;
> @@ -593,57 +592,55 @@ in_arpinput(struct mbuf *m)
> bcopy((caddr_t)ea->arp_spa, (caddr_t)&isaddr, sizeof(isaddr));
>
> /* First try: check target against our addresses */
> - TAILQ_FOREACH(ia, &in_ifaddr, ia_list) {
> - if (itaddr.s_addr != ia->ia_addr.sin_addr.s_addr)
> + TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
> + if (ifa->ifa_addr->sa_family != AF_INET)
> + continue;
> +
> + if (itaddr.s_addr != ifatoia(ifa)->ia_addr.sin_addr.s_addr)
> continue;
>
> #if NCARP > 0
> - if (ia->ia_ifp->if_type == IFT_CARP &&
> - ((ia->ia_ifp->if_flags & (IFF_UP|IFF_RUNNING)) ==
> + if (ifp->if_type == IFT_CARP &&
> + ((ifp->if_flags & (IFF_UP|IFF_RUNNING)) ==
> (IFF_UP|IFF_RUNNING))) {
> - if (ia->ia_ifp == m->m_pkthdr.rcvif) {
> - if (op == ARPOP_REPLY)
> - break;
> - if (carp_iamatch(ia, ea->arp_sha,
> - &enaddr, ðer_shost))
> - break;
> - else
> - goto out;
> - }
> - } else
> -#endif
> - if (ia->ia_ifp == m->m_pkthdr.rcvif)
> + if (op == ARPOP_REPLY)
> + break;
> + if (carp_iamatch(ifatoia(ifa), ea->arp_sha,
> + &enaddr, ðer_shost))
> break;
> + else
> + goto out;
> + }
> +#endif
> + break;
> }
>
> /* Second try: check source against our addresses */
> - if (ia == NULL) {
> - TAILQ_FOREACH(ia, &in_ifaddr, ia_list) {
> - if (isaddr.s_addr != ia->ia_addr.sin_addr.s_addr)
> + if (ifa == NULL) {
> + TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
> + if (ifa->ifa_addr->sa_family != AF_INET)
>
Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
On Wed, Aug 28, 2013 at 5:54 AM, Maxime Villard wrote:
> + /* Ensure interp is a valid, NUL-terminated string */
> + for (n = 0; n < pp->p_filesz; n++) {
> + if (interp[n] == '\0')
> + break;
> + }
> + if (n != pp->p_filesz - 1) {
> + error = ENOEXEC;
> goto bad;
> }
A more concise test would be:
if (strnlen(interp, pp->p_filesz) != pp->p_filesz - 1) {
error = ENOEXEC;
goto bad;
}
Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
On 08/28/13 16:30, Kenneth R Westerback wrote:
> On Wed, Aug 28, 2013 at 02:54:11PM +0200, Maxime Villard wrote:
>> Updated diff, with small tweaks from Andres Perera,
>> * int -> size_t, signedness issue, even if it can't be >INT_MAX
>> * NULL -> NUL
>>
>>
>> Index: exec_elf.c
>> ===
>> RCS file: /cvs/src/sys/kern/exec_elf.c,v
>> retrieving revision 1.93
>> diff -u -p -r1.93 exec_elf.c
>> --- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
>> +++ exec_elf.c 28 Aug 2013 14:36:58 -
>> @@ -516,7 +516,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
>> int error, i;
>> char *interp = NULL;
>> u_long pos = 0, phsize;
>> -size_t randomizequota = ELF_RANDOMIZE_LIMIT;
>> +size_t n, randomizequota = ELF_RANDOMIZE_LIMIT;
>>
>> if (epp->ep_hdrvalid < sizeof(Elf_Ehdr))
>> return (ENOEXEC);
>> @@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
>>
>> for (i = 0, pp = ph; i < eh->e_phnum; i++, pp++) {
>> if (pp->p_type == PT_INTERP && !interp) {
>> -if (pp->p_filesz >= MAXPATHLEN)
>> +if (pp->p_filesz >= MAXPATHLEN ||
>> +pp->p_filesz < 2)
>
> Since p_filesz includes the NUL and MAXPATHLEN includes the NUL as
> far as I know, could the comparison not be simply '> MAXPATHLEN'?
>
> In passing I note that 37 out of 749 declarations using MAXPATHLEN in the
> tree use 'char blah[MAXPATHLEN + 1]'. No idea if it is worth looking at
> making them do without the '+ 1'. :-)
>
> I also note in passing several "#define PATH_MAX (MAXPATHLEN + 1)'
> declarations, which strike me as odd since MAXPATHLEN is defined
> in sys/param.h by '#define MAXPATHEN PATH_MAX'.
>
> Let the POSIX lawyers apply any necessary cluebats please.
>
>> goto bad;
>> interp = pool_get(&namei_pool, PR_WAITOK);
>> if ((error = ELFNAME(read_from)(p, epp->ep_vp,
>> pp->p_offset, interp, pp->p_filesz)) != 0) {
>> +goto bad;
>> +}
>> +/* Ensure interp is a valid, NUL-terminated string */
>
> The condition is not that the string is NUL terminated ("aaa\0aaa\0" is after
> all NUL terminated and valid as far as any function reading strings would be
> concerned. It just has trailing garbage.) Perhaps the comment should refer
> to making sure the string is spec compliant by being of the expected length.
By 'valid string' I actually meant 'without trailing garbage'... but ok
if it's not clear
/* Ensure interp is NUL-terminated and of the expected length */
>
> Ken
>
>> +for (n = 0; n < pp->p_filesz; n++) {
>> +if (interp[n] == '\0')
>> +break;
>> +}
>> +if (n != pp->p_filesz - 1) {
>> +error = ENOEXEC;
>> goto bad;
>> }
>> } else if (pp->p_type == PT_LOAD) {
>>
>>
>>
>> On 08/28/13 08:44, Maxime Villard wrote:
>>> Hi,
>>> in the ELF format, the PT_INTERP segment contains the path of the
>>> interpreter which must be loaded. For this segment, the kernel looks
>>> at these values in the program header:
>>>
>>>p_offset: offset of the path string
>>>p_filesz: size of the path string, including the \0
>>>
>>> The path string must be a valid string, or the binary won't be loaded.
>>>
>>> The kernel actually doesn't check the string, it just reads p_filesz
>>> bytes from p_offset, and later it checks if it can be loaded. Malformed
>>> binaries which have paths like:
>>>
>>> "a"
>>> or
>>> "aaa\0aaa\0aaa\0aaa\0"
>>>
>>> are parsed, loaded, and then the kernel figures out that the path is
>>> not valid, and aborts.
>>>
>>> When the kernel reads the path from the file, it should check directly
>>> the string to ensure it is at least a valid string.
>>>
>>> Here is a patch for this. For pp->p_filesz, I could have put
>>> if (pp->p_filesz == 0)
>>> but values of 1 also don't make sense; "\0" can't be a valid path.
>>>
>>>
>>>
>>> Index: exec_elf.c
>>> ===
>>> RCS file: /cvs/src/sys/kern/exec_elf.c,v
>>> retrieving revision 1.93
>>> diff -u -p -r1.93 exec_elf.c
>>> --- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
>>> +++ exec_elf.c 27 Aug 2013 22:59:21 -
>>> @@ -513,7 +513,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
>>> Elf_Ehdr *eh = epp->ep_hdr;
>>> Elf_Phdr *ph, *pp, *base_ph = NULL;
>>> Elf_Addr phdr = 0, exe_base = 0;
>>> - int error, i;
>>> + int error, i, n;
>>> char *interp = NULL;
>>> u_long pos = 0, phsize;
>>> size_t randomizequota = ELF_RANDOMIZE_LIMIT;
>>> @@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct pro
games/trek: fix for tournament mode
trek(6)'s tournament mode is intended to have deterministic "random" numbers based on a password. This was broken 15 years ago when rand() was changed to random(), but the initial srand() was forgotten. ok? Index: setup.c === RCS file: /cvs/src/games/trek/setup.c,v retrieving revision 1.8 diff -u -p -r1.8 setup.c --- setup.c 27 Oct 2009 23:59:27 - 1.8 +++ setup.c 28 Aug 2013 17:43:54 - @@ -105,7 +105,7 @@ setup() d = 0; for (i = 0; Game.passwd[i]; i++) d += Game.passwd[i] << i; - srand(d); + srandom(d); } Param.bases = Now.bases = ranf(6 - Game.skill) + 2; if (Game.skill == 6) -- Christian "naddy" Weisgerber na...@mips.inka.de
arc4random for src/games
This replaces srandomdev()+random() with calls to arc4random*() in
src/games. There isn't much practical benefit to this. Consider
it a style fix.
I have NOT touched the games that call srandom() with a particular seed
for deterministic gameplay.
Index: arithmetic/arithmetic.c
===
RCS file: /cvs/src/games/arithmetic/arithmetic.c,v
retrieving revision 1.17
diff -u -p -r1.17 arithmetic.c
--- arithmetic/arithmetic.c 27 Oct 2009 23:59:23 - 1.17
+++ arithmetic/arithmetic.c 28 Aug 2013 14:30:16 -
@@ -124,9 +124,6 @@ main(int argc, char *argv[])
if (argc -= optind)
usage();
- /* Seed the random-number generator. */
- srandomdev();
-
(void)signal(SIGINT, intr);
/* Now ask the questions. */
@@ -177,7 +174,7 @@ problem(void)
int left, op, right, result;
char line[80];
- op = keys[random() % nkeys];
+ op = keys[arc4random_uniform(nkeys)];
if (op != '/')
right = getrandom(rangemax + 1, op, 1);
retry:
@@ -198,7 +195,7 @@ retry:
case '/':
right = getrandom(rangemax, op, 1) + 1;
result = getrandom(rangemax + 1, op, 0);
- left = right * result + random() % right;
+ left = right * result + arc4random_uniform(right);
break;
}
@@ -308,7 +305,7 @@ getrandom(int maxval, int op, int operan
struct penalty **pp, *p;
op = opnum(op);
- value = random() % (maxval + penalty[op][operand]);
+ value = arc4random_uniform(maxval + penalty[op][operand]);
/*
* 0 to maxval - 1 is a number to be used directly; bigger values
Index: backgammon/backgammon/main.c
===
RCS file: /cvs/src/games/backgammon/backgammon/main.c,v
retrieving revision 1.16
diff -u -p -r1.16 main.c
--- backgammon/backgammon/main.c27 Oct 2009 23:59:23 - 1.16
+++ backgammon/backgammon/main.c28 Aug 2013 14:34:54 -
@@ -100,7 +100,6 @@ main (argc,argv)
/* use whole screen for text */
begscr = 0;
- srandomdev(); /* seed random number generator */
getarg(argc, argv);
args[acnt] = '\0';
Index: backgammon/common_source/back.h
===
RCS file: /cvs/src/games/backgammon/common_source/back.h,v
retrieving revision 1.11
diff -u -p -r1.11 back.h
--- backgammon/common_source/back.h 14 Dec 2006 10:14:05 - 1.11
+++ backgammon/common_source/back.h 28 Aug 2013 14:35:11 -
@@ -43,7 +43,7 @@
#include
#include
-#define rnum(r)(random()%r)
+#define rnum(r)arc4random_uniform(r)
#define D0 dice[0]
#define D1 dice[1]
#define swap {D0 ^= D1; D1 ^= D0; D0 ^= D1; d0 = 1-d0;}
Index: battlestar/extern.h
===
RCS file: /cvs/src/games/battlestar/extern.h,v
retrieving revision 1.14
diff -u -p -r1.14 extern.h
--- battlestar/extern.h 5 Apr 2013 01:28:27 - 1.14
+++ battlestar/extern.h 28 Aug 2013 14:36:46 -
@@ -49,7 +49,7 @@
#define BITS (8 * sizeof (int))
#define OUTSIDE(position > 68 && position < 246 && position !=
218)
-#define rnd(x) (random() % (x))
+#define rnd(x) arc4random_uniform(x)
#define max(a,b) ((a) < (b) ? (b) : (a))
/* avoid name collision with sys/param.h */
#define TestBit(array, index) (array[index/BITS] & (1 << (index % BITS)))
Index: battlestar/init.c
===
RCS file: /cvs/src/games/battlestar/init.c,v
retrieving revision 1.13
diff -u -p -r1.13 init.c
--- battlestar/init.c 27 Oct 2009 23:59:24 - 1.13
+++ battlestar/init.c 28 Aug 2013 14:37:03 -
@@ -46,7 +46,6 @@ initialize(const char *filename)
puts("First Adventure game written by His Lordship, the honorable");
puts("Admiral D.W. Riggle\n");
location = dayfile;
- srandomdev();
username = getutmp();
if (username == NULL)
errx(1, "Don't know who you are.");
Index: bs/bs.c
===
RCS file: /cvs/src/games/bs/bs.c,v
retrieving revision 1.23
diff -u -p -r1.23 bs.c
--- bs/bs.c 14 Nov 2009 02:20:43 - 1.23
+++ bs/bs.c 28 Aug 2013 14:39:39 -
@@ -39,8 +39,6 @@
* v2.2 with bugfixes and strategical improvements, March 1998.
*/
-/* #define _POSIX_SOURCE */ /* ( random() ) */
-
#include
#include
#include
@@ -235,8 +233,6 @@ static void intro(void)
{
char *tmpname;
-srandomdev(); /* Kick the random number generator */
-
(void) signal(SIGINT,uninitgame);
(void) signal(SIGINT,uninitgame);
if(signal(SIGQUIT,SIG_IGN) != SIG_IGN)
@@ -344,7 +340,7 @@ static void placeship(int b, shi
Re: missing bit in audio.4 ?
On Wed, Aug 28, 2013 at 09:27:59AM +0200, Remco wrote: > Index: audio.4 > === > RCS file: /home/cvs/src/share/man/man4/audio.4,v > retrieving revision 1.62 > diff -u -r1.62 audio.4 > --- audio.4 15 Jul 2010 03:43:11 - 1.62 > +++ audio.4 28 Aug 2013 07:22:57 - > @@ -154,7 +154,7 @@ > .Va buffer_size > (as available via > .Dv AUDIO_GETINFO ) . > -The device driver will continuously move data from this buffer > +The device driver will continuously move data to/from this buffer > from/to the audio hardware, wrapping around at the end of the buffer. > To find out where the hardware is currently accessing data in the buffer the > .Dv AUDIO_GETIOFFS > i've fixed this by rewording it a bit, so that it reads better. i sat there thinking 2xto/from is a weird construction...there's gotta be a better way ;) jmc Index: audio.4 === RCS file: /cvs/src/share/man/man4/audio.4,v retrieving revision 1.62 diff -u -r1.62 audio.4 --- audio.4 15 Jul 2010 03:43:11 - 1.62 +++ audio.4 28 Aug 2013 16:18:35 - @@ -154,8 +154,8 @@ .Va buffer_size (as available via .Dv AUDIO_GETINFO ) . -The device driver will continuously move data from this buffer -from/to the audio hardware, wrapping around at the end of the buffer. +The device driver will continuously move data between this buffer +and the audio hardware, wrapping around at the end of the buffer. To find out where the hardware is currently accessing data in the buffer the .Dv AUDIO_GETIOFFS and
Re: threaded prof signals
On Fri, Aug 16, 2013 at 02:12, Ted Unangst wrote:
> As per http://research.swtch.com/macpprof
>
> We deliver all prof signals to the main thread, which is unlikely to
> result in accurate profiling info. I think the diff below fixes things.
floating this again. note that it's not the clock being made per
thread, just the timeout, which is only used to move signal delivery
from hardclock to softclock.
>
> Index: kern/kern_clock.c
> ===
> RCS file: /cvs/src/sys/kern/kern_clock.c,v
> retrieving revision 1.81
> diff -u -p -r1.81 kern_clock.c
> --- kern/kern_clock.c 24 Apr 2013 17:29:02 - 1.81
> +++ kern/kern_clock.c 16 Aug 2013 05:57:27 -
> @@ -173,9 +173,9 @@ virttimer_trampoline(void *v)
> void
> proftimer_trampoline(void *v)
> {
> - struct process *pr = v;
> + struct proc *p = v;
>
> - psignal(pr->ps_mainproc, SIGPROF);
> + psignal(p, SIGPROF);
> }
>
> /*
> @@ -200,7 +200,7 @@ hardclock(struct clockframe *frame)
> timeout_add(&pr->ps_virt_to, 1);
> if (timerisset(&pr->ps_timer[ITIMER_PROF].it_value) &&
> itimerdecr(&pr->ps_timer[ITIMER_PROF], tick) == 0)
> - timeout_add(&pr->ps_prof_to, 1);
> + timeout_add(&p->p_prof_to, 1);
> }
>
> /*
> Index: kern/kern_exit.c
> ===
> RCS file: /cvs/src/sys/kern/kern_exit.c,v
> retrieving revision 1.125
> diff -u -p -r1.125 kern_exit.c
> --- kern/kern_exit.c 5 Jun 2013 00:53:26 - 1.125
> +++ kern/kern_exit.c 16 Aug 2013 05:57:27 -
> @@ -183,10 +183,10 @@ exit1(struct proc *p, int rv, int flags)
> */
> fdfree(p);
>
> + timeout_del(&p->p_prof_to);
> if ((p->p_flag & P_THREAD) == 0) {
> timeout_del(&pr->ps_realit_to);
> timeout_del(&pr->ps_virt_to);
> - timeout_del(&pr->ps_prof_to);
> #ifdef SYSVSEM
> semexit(pr);
> #endif
> Index: kern/kern_fork.c
> ===
> RCS file: /cvs/src/sys/kern/kern_fork.c,v
> retrieving revision 1.152
> diff -u -p -r1.152 kern_fork.c
> --- kern/kern_fork.c 11 Jun 2013 13:00:31 - 1.152
> +++ kern/kern_fork.c 16 Aug 2013 05:57:27 -
> @@ -209,7 +209,6 @@ process_new(struct proc *p, struct proce
>
> timeout_set(&pr->ps_realit_to, realitexpire, pr);
> timeout_set(&pr->ps_virt_to, virttimer_trampoline, pr);
> - timeout_set(&pr->ps_prof_to, proftimer_trampoline, pr);
>
> pr->ps_flags = parent->ps_flags & (PS_SUGID | PS_SUGIDEXEC);
> if (parent->ps_session->s_ttyvp != NULL &&
> @@ -342,6 +341,7 @@ fork1(struct proc *curp, int exitsig, in
> * Initialize the timeouts.
> */
> timeout_set(&p->p_sleep_to, endtsleep, p);
> + timeout_set(&p->p_prof_to, proftimer_trampoline, p);
>
> /*
> * Duplicate sub-structures as needed.
> Index: kern/kern_time.c
> ===
> RCS file: /cvs/src/sys/kern/kern_time.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 kern_time.c
> --- kern/kern_time.c 17 Jun 2013 19:11:54 - 1.80
> +++ kern/kern_time.c 16 Aug 2013 05:57:27 -
> @@ -568,7 +568,7 @@ sys_setitimer(struct proc *p, void *v, r
> if (which == ITIMER_VIRTUAL)
> timeout_del(&pr->ps_virt_to);
> if (which == ITIMER_PROF)
> - timeout_del(&pr->ps_prof_to);
> + timeout_del(&p->p_prof_to);
> splx(s);
> }
>
> Index: sys/proc.h
> ===
> RCS file: /cvs/src/sys/sys/proc.h,v
> retrieving revision 1.168
> diff -u -p -r1.168 proc.h
> --- sys/proc.h6 Jun 2013 13:09:37 - 1.168
> +++ sys/proc.h16 Aug 2013 05:57:28 -
> @@ -210,7 +210,6 @@ struct process {
> structtimespec ps_start; /* starting time. */
> structtimeout ps_realit_to; /* real-time itimer trampoline. */
> structtimeout ps_virt_to; /* virtual itimer trampoline. */
> - struct timeout ps_prof_to; /* prof itimer trampoline. */
>
> int ps_refcnt; /* Number of references. */
> };
> @@ -337,6 +336,7 @@ struct proc {
> /* End area that is copied on creation. */
> #define p_endcopy p_addr
>
> + struct timeout p_prof_to; /* prof itimer trampoline. */
> structuser *p_addr; /* Kernel virtual addr of u-area */
> structmdproc p_md;/* Any machine-dependent fields. */
Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
On Wed, Aug 28, 2013 at 02:54:11PM +0200, Maxime Villard wrote:
> Updated diff, with small tweaks from Andres Perera,
> * int -> size_t, signedness issue, even if it can't be >INT_MAX
> * NULL -> NUL
>
>
> Index: exec_elf.c
> ===
> RCS file: /cvs/src/sys/kern/exec_elf.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 exec_elf.c
> --- exec_elf.c4 Jul 2013 17:37:05 - 1.93
> +++ exec_elf.c28 Aug 2013 14:36:58 -
> @@ -516,7 +516,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
> int error, i;
> char *interp = NULL;
> u_long pos = 0, phsize;
> - size_t randomizequota = ELF_RANDOMIZE_LIMIT;
> + size_t n, randomizequota = ELF_RANDOMIZE_LIMIT;
>
> if (epp->ep_hdrvalid < sizeof(Elf_Ehdr))
> return (ENOEXEC);
> @@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
>
> for (i = 0, pp = ph; i < eh->e_phnum; i++, pp++) {
> if (pp->p_type == PT_INTERP && !interp) {
> - if (pp->p_filesz >= MAXPATHLEN)
> + if (pp->p_filesz >= MAXPATHLEN ||
> + pp->p_filesz < 2)
Since p_filesz includes the NUL and MAXPATHLEN includes the NUL as
far as I know, could the comparison not be simply '> MAXPATHLEN'?
In passing I note that 37 out of 749 declarations using MAXPATHLEN in the
tree use 'char blah[MAXPATHLEN + 1]'. No idea if it is worth looking at
making them do without the '+ 1'. :-)
I also note in passing several "#define PATH_MAX (MAXPATHLEN + 1)'
declarations, which strike me as odd since MAXPATHLEN is defined
in sys/param.h by '#define MAXPATHEN PATH_MAX'.
Let the POSIX lawyers apply any necessary cluebats please.
> goto bad;
> interp = pool_get(&namei_pool, PR_WAITOK);
> if ((error = ELFNAME(read_from)(p, epp->ep_vp,
> pp->p_offset, interp, pp->p_filesz)) != 0) {
> + goto bad;
> + }
> + /* Ensure interp is a valid, NUL-terminated string */
The condition is not that the string is NUL terminated ("aaa\0aaa\0" is after
all NUL terminated and valid as far as any function reading strings would be
concerned. It just has trailing garbage.) Perhaps the comment should refer
to making sure the string is spec compliant by being of the expected length.
Ken
> + for (n = 0; n < pp->p_filesz; n++) {
> + if (interp[n] == '\0')
> + break;
> + }
> + if (n != pp->p_filesz - 1) {
> + error = ENOEXEC;
> goto bad;
> }
> } else if (pp->p_type == PT_LOAD) {
>
>
>
> On 08/28/13 08:44, Maxime Villard wrote:
> > Hi,
> > in the ELF format, the PT_INTERP segment contains the path of the
> > interpreter which must be loaded. For this segment, the kernel looks
> > at these values in the program header:
> >
> > p_offset: offset of the path string
> > p_filesz: size of the path string, including the \0
> >
> > The path string must be a valid string, or the binary won't be loaded.
> >
> > The kernel actually doesn't check the string, it just reads p_filesz
> > bytes from p_offset, and later it checks if it can be loaded. Malformed
> > binaries which have paths like:
> >
> > "a"
> > or
> > "aaa\0aaa\0aaa\0aaa\0"
> >
> > are parsed, loaded, and then the kernel figures out that the path is
> > not valid, and aborts.
> >
> > When the kernel reads the path from the file, it should check directly
> > the string to ensure it is at least a valid string.
> >
> > Here is a patch for this. For pp->p_filesz, I could have put
> > if (pp->p_filesz == 0)
> > but values of 1 also don't make sense; "\0" can't be a valid path.
> >
> >
> >
> > Index: exec_elf.c
> > ===
> > RCS file: /cvs/src/sys/kern/exec_elf.c,v
> > retrieving revision 1.93
> > diff -u -p -r1.93 exec_elf.c
> > --- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
> > +++ exec_elf.c 27 Aug 2013 22:59:21 -
> > @@ -513,7 +513,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
> > Elf_Ehdr *eh = epp->ep_hdr;
> > Elf_Phdr *ph, *pp, *base_ph = NULL;
> > Elf_Addr phdr = 0, exe_base = 0;
> > - int error, i;
> > + int error, i, n;
> > char *interp = NULL;
> > u_long pos = 0, phsize;
> > size_t randomizequota = ELF_RANDOMIZE_LIMIT;
> > @@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
> >
> > for (i = 0, pp = ph; i < eh->e_phnum; i++, pp++) {
> > if (pp->p_type == PT_INTERP && !interp) {
> > - if (pp->p_filesz >= MAXPATHLEN)
> > + if (pp->p_filesz >= MAXPATHLEN ||
> > +
Don't iterate on the global list in arp_{request,input}
Like the previous diffs, when we already have the ifp and want one of
its addresses, iterate on the ifp list instead of the global one.
Tested with carp here. I appreciate any comment and oks.
Index: netinet/if_ether.c
===
RCS file: /cvs/src/sys/netinet/if_ether.c,v
retrieving revision 1.105
diff -u -p -r1.105 if_ether.c
--- netinet/if_ether.c 28 Aug 2013 06:58:57 - 1.105
+++ netinet/if_ether.c 28 Aug 2013 07:57:01 -
@@ -145,7 +145,6 @@ arp_rtrequest(int req, struct rtentry *r
{
struct sockaddr *gate = rt->rt_gateway;
struct llinfo_arp *la = (struct llinfo_arp *)rt->rt_llinfo;
- struct in_ifaddr *ia;
struct ifaddr *ifa;
struct mbuf *m;
@@ -259,13 +258,13 @@ arp_rtrequest(int req, struct rtentry *r
rt->rt_flags |= RTF_LLINFO;
LIST_INSERT_HEAD(&llinfo_arp, la, la_list);
- TAILQ_FOREACH(ia, &in_ifaddr, ia_list) {
- if (ia->ia_ifp == rt->rt_ifp &&
- SIN(rt_key(rt))->sin_addr.s_addr ==
- (IA_SIN(ia))->sin_addr.s_addr)
+ TAILQ_FOREACH(ifa, &rt->rt_ifp->if_addrlist, ifa_list) {
+ if ((ifa->ifa_addr->sa_family == AF_INET) &&
+ ifatoia(ifa)->ia_addr.sin_addr.s_addr ==
+ satosin(rt_key(rt))->sin_addr.s_addr)
break;
}
- if (ia) {
+ if (ifa) {
/*
* This test used to be
* if (lo0ifp->if_flags & IFF_UP)
@@ -294,7 +293,6 @@ arp_rtrequest(int req, struct rtentry *r
* address we are using, otherwise we will have trouble
* with source address selection.
*/
- ifa = &ia->ia_ifa;
if (ifa != rt->rt_ifa) {
ifafree(rt->rt_ifa);
ifa->ifa_refcnt++;
@@ -562,11 +560,12 @@ void
in_arpinput(struct mbuf *m)
{
struct ether_arp *ea;
- struct arpcom *ac = (struct arpcom *)m->m_pkthdr.rcvif;
+ struct ifnet *ifp = m->m_pkthdr.rcvif;
+ struct arpcom *ac = (struct arpcom *)ifp;
struct ether_header *eh;
struct llinfo_arp *la = 0;
struct rtentry *rt;
- struct in_ifaddr *ia;
+ struct ifaddr *ifa;
struct sockaddr_dl *sdl;
struct sockaddr sa;
struct in_addr isaddr, itaddr, myaddr;
@@ -593,57 +592,55 @@ in_arpinput(struct mbuf *m)
bcopy((caddr_t)ea->arp_spa, (caddr_t)&isaddr, sizeof(isaddr));
/* First try: check target against our addresses */
- TAILQ_FOREACH(ia, &in_ifaddr, ia_list) {
- if (itaddr.s_addr != ia->ia_addr.sin_addr.s_addr)
+ TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
+ if (ifa->ifa_addr->sa_family != AF_INET)
+ continue;
+
+ if (itaddr.s_addr != ifatoia(ifa)->ia_addr.sin_addr.s_addr)
continue;
#if NCARP > 0
- if (ia->ia_ifp->if_type == IFT_CARP &&
- ((ia->ia_ifp->if_flags & (IFF_UP|IFF_RUNNING)) ==
+ if (ifp->if_type == IFT_CARP &&
+ ((ifp->if_flags & (IFF_UP|IFF_RUNNING)) ==
(IFF_UP|IFF_RUNNING))) {
- if (ia->ia_ifp == m->m_pkthdr.rcvif) {
- if (op == ARPOP_REPLY)
- break;
- if (carp_iamatch(ia, ea->arp_sha,
- &enaddr, ðer_shost))
- break;
- else
- goto out;
- }
- } else
-#endif
- if (ia->ia_ifp == m->m_pkthdr.rcvif)
+ if (op == ARPOP_REPLY)
+ break;
+ if (carp_iamatch(ifatoia(ifa), ea->arp_sha,
+ &enaddr, ðer_shost))
break;
+ else
+ goto out;
+ }
+#endif
+ break;
}
/* Second try: check source against our addresses */
- if (ia == NULL) {
- TAILQ_FOREACH(ia, &in_ifaddr, ia_list) {
- if (isaddr.s_addr != ia->ia_addr.sin_addr.s_addr)
+ if (ifa == NULL) {
+ TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
+ if (ifa->ifa_addr->sa_family != AF_INET)
continue;
- if (ia->ia_ifp == m->m_pkthdr.rcvif)
+
+ if (isaddr.s_addr ==
+ ifatoia(ifa)->ia_addr.sin_addr.s_addr)
Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
Updated diff, with small tweaks from Andres Perera,
* int -> size_t, signedness issue, even if it can't be >INT_MAX
* NULL -> NUL
Index: exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -p -r1.93 exec_elf.c
--- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
+++ exec_elf.c 28 Aug 2013 14:36:58 -
@@ -516,7 +516,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
int error, i;
char *interp = NULL;
u_long pos = 0, phsize;
- size_t randomizequota = ELF_RANDOMIZE_LIMIT;
+ size_t n, randomizequota = ELF_RANDOMIZE_LIMIT;
if (epp->ep_hdrvalid < sizeof(Elf_Ehdr))
return (ENOEXEC);
@@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
for (i = 0, pp = ph; i < eh->e_phnum; i++, pp++) {
if (pp->p_type == PT_INTERP && !interp) {
- if (pp->p_filesz >= MAXPATHLEN)
+ if (pp->p_filesz >= MAXPATHLEN ||
+ pp->p_filesz < 2)
goto bad;
interp = pool_get(&namei_pool, PR_WAITOK);
if ((error = ELFNAME(read_from)(p, epp->ep_vp,
pp->p_offset, interp, pp->p_filesz)) != 0) {
+ goto bad;
+ }
+ /* Ensure interp is a valid, NUL-terminated string */
+ for (n = 0; n < pp->p_filesz; n++) {
+ if (interp[n] == '\0')
+ break;
+ }
+ if (n != pp->p_filesz - 1) {
+ error = ENOEXEC;
goto bad;
}
} else if (pp->p_type == PT_LOAD) {
On 08/28/13 08:44, Maxime Villard wrote:
> Hi,
> in the ELF format, the PT_INTERP segment contains the path of the
> interpreter which must be loaded. For this segment, the kernel looks
> at these values in the program header:
>
> p_offset: offset of the path string
> p_filesz: size of the path string, including the \0
>
> The path string must be a valid string, or the binary won't be loaded.
>
> The kernel actually doesn't check the string, it just reads p_filesz
> bytes from p_offset, and later it checks if it can be loaded. Malformed
> binaries which have paths like:
>
> "a"
> or
> "aaa\0aaa\0aaa\0aaa\0"
>
> are parsed, loaded, and then the kernel figures out that the path is
> not valid, and aborts.
>
> When the kernel reads the path from the file, it should check directly
> the string to ensure it is at least a valid string.
>
> Here is a patch for this. For pp->p_filesz, I could have put
> if (pp->p_filesz == 0)
> but values of 1 also don't make sense; "\0" can't be a valid path.
>
>
>
> Index: exec_elf.c
> ===
> RCS file: /cvs/src/sys/kern/exec_elf.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 exec_elf.c
> --- exec_elf.c4 Jul 2013 17:37:05 - 1.93
> +++ exec_elf.c27 Aug 2013 22:59:21 -
> @@ -513,7 +513,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
> Elf_Ehdr *eh = epp->ep_hdr;
> Elf_Phdr *ph, *pp, *base_ph = NULL;
> Elf_Addr phdr = 0, exe_base = 0;
> - int error, i;
> + int error, i, n;
> char *interp = NULL;
> u_long pos = 0, phsize;
> size_t randomizequota = ELF_RANDOMIZE_LIMIT;
> @@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
>
> for (i = 0, pp = ph; i < eh->e_phnum; i++, pp++) {
> if (pp->p_type == PT_INTERP && !interp) {
> - if (pp->p_filesz >= MAXPATHLEN)
> + if (pp->p_filesz >= MAXPATHLEN ||
> + pp->p_filesz < 2)
> goto bad;
> interp = pool_get(&namei_pool, PR_WAITOK);
> if ((error = ELFNAME(read_from)(p, epp->ep_vp,
> pp->p_offset, interp, pp->p_filesz)) != 0) {
> + goto bad;
> + }
> + /* Ensure interp is a valid, NULL-terminated string */
> + for (n = 0; n < pp->p_filesz; n++) {
> + if (interp[n] == '\0')
> + break;
> + }
> + if (n != pp->p_filesz - 1) {
> + error = ENOEXEC;
> goto bad;
> }
> } else if (pp->p_type == PT_LOAD) {
>
>
>
> If you want to test
> * Before patching
> take whatever binary you have, open it with a text/hex editor, at the
> beginning of the file there should be a string like "/usr/libexec/ld.so",
> just add a charac
Re: OpenBSD on a Nokia IP380
charter.net> writes: > installed. Guess I'll have to live with the 4 onboard NICs for the time > being. If you have no plans to forward full-blown 100Mbps between fxps and there's a dot1q-aware switch near you, using vlans may be a workaround for a limited number of physical ports.
missing bit in audio.4 ?
Index: audio.4 === RCS file: /home/cvs/src/share/man/man4/audio.4,v retrieving revision 1.62 diff -u -r1.62 audio.4 --- audio.4 15 Jul 2010 03:43:11 - 1.62 +++ audio.4 28 Aug 2013 07:22:57 - @@ -154,7 +154,7 @@ .Va buffer_size (as available via .Dv AUDIO_GETINFO ) . -The device driver will continuously move data from this buffer +The device driver will continuously move data to/from this buffer from/to the audio hardware, wrapping around at the end of the buffer. To find out where the hardware is currently accessing data in the buffer the .Dv AUDIO_GETIOFFS
[PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
Hi,
in the ELF format, the PT_INTERP segment contains the path of the
interpreter which must be loaded. For this segment, the kernel looks
at these values in the program header:
p_offset: offset of the path string
p_filesz: size of the path string, including the \0
The path string must be a valid string, or the binary won't be loaded.
The kernel actually doesn't check the string, it just reads p_filesz
bytes from p_offset, and later it checks if it can be loaded. Malformed
binaries which have paths like:
"a"
or
"aaa\0aaa\0aaa\0aaa\0"
are parsed, loaded, and then the kernel figures out that the path is
not valid, and aborts.
When the kernel reads the path from the file, it should check directly
the string to ensure it is at least a valid string.
Here is a patch for this. For pp->p_filesz, I could have put
if (pp->p_filesz == 0)
but values of 1 also don't make sense; "\0" can't be a valid path.
Index: exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -p -r1.93 exec_elf.c
--- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
+++ exec_elf.c 27 Aug 2013 22:59:21 -
@@ -513,7 +513,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
Elf_Ehdr *eh = epp->ep_hdr;
Elf_Phdr *ph, *pp, *base_ph = NULL;
Elf_Addr phdr = 0, exe_base = 0;
- int error, i;
+ int error, i, n;
char *interp = NULL;
u_long pos = 0, phsize;
size_t randomizequota = ELF_RANDOMIZE_LIMIT;
@@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
for (i = 0, pp = ph; i < eh->e_phnum; i++, pp++) {
if (pp->p_type == PT_INTERP && !interp) {
- if (pp->p_filesz >= MAXPATHLEN)
+ if (pp->p_filesz >= MAXPATHLEN ||
+ pp->p_filesz < 2)
goto bad;
interp = pool_get(&namei_pool, PR_WAITOK);
if ((error = ELFNAME(read_from)(p, epp->ep_vp,
pp->p_offset, interp, pp->p_filesz)) != 0) {
+ goto bad;
+ }
+ /* Ensure interp is a valid, NULL-terminated string */
+ for (n = 0; n < pp->p_filesz; n++) {
+ if (interp[n] == '\0')
+ break;
+ }
+ if (n != pp->p_filesz - 1) {
+ error = ENOEXEC;
goto bad;
}
} else if (pp->p_type == PT_LOAD) {
If you want to test
* Before patching
take whatever binary you have, open it with a text/hex editor, at the
beginning of the file there should be a string like "/usr/libexec/ld.so",
just add a character to it, then run the binary with execv(): you will
get abort traps.
* After patching
execv() now returns -1, and errno is set to ENOEXEC.
There should not be any regression; if there is, it means that the binary
is not spec compliant.
Ok/Comments?
