Date: Sat, 21 Mar 2015 21:05:00 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Tiwei Bie <btw@mail.ustc.edu.cn> Cc: freebsd-hackers@freebsd.org Subject: Re: [PATCH] Finish the task 'Validate coredump format string' Message-ID: <20150321200500.GC14650@dft-labs.eu> In-Reply-To: <1426946345-67889-1-git-send-email-btw@mail.ustc.edu.cn> References: <1426946345-67889-1-git-send-email-btw@mail.ustc.edu.cn>
next in thread | previous in thread | raw e-mail | index | archive | help
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?20150321200500.GC14650>