Date: Sun, 22 Mar 2015 17:19:40 +0800 From: Tiwei Bie <btw@mail.ustc.edu.cn> To: Mateusz Guzik <mjguzik@gmail.com> Cc: Konstantin Belousov <kostikbel@gmail.com>, freebsd-hackers@freebsd.org Subject: Re: [PATCH] Finish the task 'Validate coredump format string' Message-ID: <20150322091853.GA89976@freebsd> In-Reply-To: <20150321200500.GC14650@dft-labs.eu> References: <1426946345-67889-1-git-send-email-btw@mail.ustc.edu.cn> <20150321200500.GC14650@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
Sorry, I introduced a bug... allproc_lock could not be used to protect
the access to corefilename[].
Because, sysctl_kern_corefile() could be called very early:
static void
sysctl_register_all(void *arg)
{
struct sysctl_oid **oidp;
sx_init(&sysctlmemlock, "sysctl mem");
SYSCTL_INIT();
SYSCTL_XLOCK();
SET_FOREACH(oidp, sysctl_set)
sysctl_register_oid(*oidp);
SYSCTL_XUNLOCK();
}
SYSINIT(sysctl, SI_SUB_KMEM, SI_ORDER_FIRST, sysctl_register_all, 0);
That is to say, when the tunable `kern.corefile' is set in loader.conf,
sysctl_kern_corefile() will be called as the priority of (SI_SUB_KMEM,
SI_ORDER_FIRST).
At this time, allproc_lock is not initialized.
allproc_lock is initialized as the priority of (SI_SUB_INTRINSIC,
SI_ORDER_FIRST):
void
procinit()
{
......
sx_init(&allproc_lock, "allproc");
......
}
static void
proc0_init(void *dummy __unused)
{
......
procinit();
......
}
SYSINIT(p0init, SI_SUB_INTRINSIC, SI_ORDER_FIRST, proc0_init, NULL);
Sorry...
I couldn't find a proper existing lock for this task. Maybe a dedicated
lock needs to be created. And initialize it together with sysctlmemlock:
static void
sysctl_register_all(void *arg)
{
struct sysctl_oid **oidp;
sx_init(&sysctlmemlock, "sysctl mem");
SYSCTL_INIT();
SYSCTL_XLOCK();
SET_FOREACH(oidp, sysctl_set)
sysctl_register_oid(*oidp);
SYSCTL_XUNLOCK();
}
Or maybe sysctlmemlock could be used, which is only acuqired when
req.oldlen > PAGE_SIZE.
On Sat, Mar 21, 2015 at 09:05:00PM +0100, Mateusz Guzik wrote:
> On Sat, Mar 21, 2015 at 09:59:05PM +0800, Tiwei Bie wrote:
> > Hi, Mateusz!
> >
> > I have finished the task: Validate coredump format string [1].
> >
> > Following is my patch:
> >
> > ---
> > sys/kern/kern_sig.c | 31 ++++++++++++++++++++++++++++---
> > 1 file changed, 28 insertions(+), 3 deletions(-)
> >
> > diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
> > index 8410d9d..52f05be 100644
> > --- a/sys/kern/kern_sig.c
> > +++ b/sys/kern/kern_sig.c
> > @@ -3099,13 +3099,38 @@ static char corefilename[MAXPATHLEN] = {"%N.core"};
> > static int
> > sysctl_kern_corefile(SYSCTL_HANDLER_ARGS)
> > {
> > - int error;
> > + char *format;
> > + int i, error;
> > +
> > + format = malloc(MAXPATHLEN, M_TEMP, M_WAITOK);
> > +
> > + sx_slock(&corefilename_lock);
> > + strncpy(format, corefilename, MAXPATHLEN);
> > + sx_sunlock(&corefilename_lock);
> > +
> > + error = sysctl_handle_string(oidp, format, MAXPATHLEN, req);
> > + if (error != 0 || strcmp(format, corefilename) == 0)
> > + goto out;
> > +
> > + for (i = 0; format[i] != '\0'; i++) {
> > + if (format[i] == '%') {
> > + char ch = format[++i];
> > + if (ch != '%' && ch != 'H' && ch != 'I' &&
> > + ch != 'N' && ch != 'P' && ch != 'U') {
> > + error = EINVAL;
> > + printf("Unknown format character %c in "
> > + "corename `%s'\n", ch, format);
> > + goto out;
> > + }
> > + }
> > + }
>
> Code traversing the string uses 'switch'. Any reason to deviate from that?
>
> It also uses log(LOG_ERR,) so why is printf is used here?
>
> corefilename can be also set with a bootloader tunable, so we have to
> validate what is being passed there and possibly reject it.
>
> When we know that the string we have set in corefilename is valid, there
> is no reason to have aforementioned log() in corefile_open().
>
> As a side note 'I' more than once in the format is not really supported,
> so I would check for that too.
>
> >
> > sx_xlock(&corefilename_lock);
> > - error = sysctl_handle_string(oidp, corefilename, sizeof(corefilename),
> > - req);
> > + strncpy(corefilename, format, sizeof(corefilename));
> > sx_xunlock(&corefilename_lock);
> >
> > +out:
> > + free(format, M_TEMP);
> > return (error);
> > }
> > SYSCTL_PROC(_kern, OID_AUTO, corefile, CTLTYPE_STRING | CTLFLAG_RWTUN |
> > --
> > 2.1.2
> >
> > [1] https://wiki.freebsd.org/JuniorJobs#Validate_coredump_format_string
> >
> > Best regards,
> > Tiwei Bie
> >
>
> --
> Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150322091853.GA89976>