From owner-freebsd-hackers@FreeBSD.ORG Sun Mar 22 09:20:11 2015 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 5CD9117E for ; Sun, 22 Mar 2015 09:20:11 +0000 (UTC) Received: from ustc.edu.cn (email6.ustc.edu.cn [IPv6:2001:da8:d800::8]) by mx1.freebsd.org (Postfix) with ESMTP id A8D5D6E6 for ; Sun, 22 Mar 2015 09:20:10 +0000 (UTC) Received: from freebsd (unknown [58.211.218.74]) by newmailweb.ustc.edu.cn (Coremail) with SMTP id LkAmygCniBU9iQ5V_MnCAw--.41901S2; Sun, 22 Mar 2015 17:20:04 +0800 (CST) Date: Sun, 22 Mar 2015 17:19:40 +0800 From: Tiwei Bie To: Mateusz Guzik Subject: Re: [PATCH] Finish the task 'Validate coredump format string' Message-ID: <20150322091853.GA89976@freebsd> References: <1426946345-67889-1-git-send-email-btw@mail.ustc.edu.cn> <20150321200500.GC14650@dft-labs.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150321200500.GC14650@dft-labs.eu> User-Agent: Mutt/1.5.23 (2014-03-12) X-CM-TRANSID: LkAmygCniBU9iQ5V_MnCAw--.41901S2 X-Coremail-Antispam: 1UD129KBjvJXoWxWryUCF47JF4xJr47Jr1UZFb_yoWrXFW8pF 98uFW5JrZ8Wr43Zrsav3y5ZFyFyr4kKr4Uu39xAr4ayr10qryrXr1rGryF9Fs7Wrs2gFn5 Za15Xry7K3yUZFJanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUyEb7Iv0xC_Kw4lb4IE77IF4wAFF20E14v26r1j6r4UM7CY07I2 0VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rw A2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Xr0_Ar1l84ACjcxK6xII jxv20xvEc7CjxVAFwI0_Gr0_Cr1l84ACjcxK6I8E87Iv67AKxVW0oVCq3wA2z4x0Y4vEx4 A2jsIEc7CjxVAFwI0_GcCE3s1le2I262IYc4CY6c8Ij28IcVAaY2xG8wAqx4xG64xvF2IE w4CE5I8CrVC2j2WlYx0E2Ix0cI8IcVAFwI0_JrI_JrylYx0Ex4A2jsIE14v26r4j6F4UMc vjeVCFs4IE7xkEbVWUJVW8JwACjcxG0xvEwIxGrwCY02Avz4vE14v_GFWl42xK82IYc2Ij 64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxVAqx4xG67AKxVWUJVWUGwC20s026x 8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r1Y6r17MIIYrxkI7VAKI48JMIIF0xvE 2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF7I0E14v26r1j6r4UMIIF0xvE42 xK8VAvwI8IcIk0rVWrZr1j6s0DMIIF0xvEx4A2jsIE14v26r1j6r4UMIIF0xvEx4A2jsIE c7CjxVAFwI0_Jr0_GrUvcSsGvfC2KfnxnUUI43ZEXa7IU5t8n7UUUUU== X-CM-SenderInfo: xewzqzxdloh3xvwfhvlgxou0/1tbiAQUQAVQhl-peFAAAsH Cc: Konstantin Belousov , freebsd-hackers@freebsd.org X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 22 Mar 2015 09:20:11 -0000 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