[PATCH] Finish the task 'Validate coredump format string'

[Tiwei Bie]

On Sun, Mar 22, 2015 at 01:38:22PM +0200, Konstantin Belousov wrote:
> On Sun, Mar 22, 2015 at 07:25:55PM +0800, Tiwei Bie wrote:
> > On Sun, Mar 22, 2015 at 11:14:01AM +0100, Mateusz Guzik wrote:
> > > On Sun, Mar 22, 2015 at 05:19:40PM +0800, Tiwei Bie wrote:
> > [..]
> > > 
> > > A dedicated sysinit func could fetch and validate the tunable, if any.
> > > If no tunable was provided it would alloc memory for the default.
> > > 
> > 
> > My new patch, that uses a dedicated sysinit func to fetch and validate
> > the tunable:
> > 
> > ---
> >  sys/kern/kern_sig.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 59 insertions(+), 1 deletion(-)
> > 
> > diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
> > index 8410d9d..e2a00ba 100644
> > --- a/sys/kern/kern_sig.c
> > +++ b/sys/kern/kern_sig.c
> > @@ -3096,19 +3096,77 @@ static int compress_user_cores = 0;
> >  
> >  static char corefilename[MAXPATHLEN] = {"%N.core"};
> Are {} around string literal needed ?
> 

It is not needed. But I didn't modify this line.

> >  
> > +static bool
> > +corefilename_check(const char *format)
> > +{
> > +	int i, counti;
> > +
> > +	counti = 0;
> > +	for (i = 0; format[i] != '\0'; i++) {
> > +		if (format[i] == '%') {
> > +			i++;
> > +			switch (format[i]) {
> > +			case 'I':
> > +				counti++;
> > +				if (counti > 1) {
> > +					printf("Too many index flags specified "
> > +					    "in corename `%s'\n", format);
> I very much dislike the kernel lecturing the user about things.
> Kernel must silently return an error code, with man pages providing
> explanation of errors.
> 

I will remove it. I was not sure about it. Thank you very much for telling
me about this!

> > +					return (false);
> > +				}
> > +			case '%':
> > +			case 'H':
> > +			case 'N':
> > +			case 'P':
> > +			case 'U':
> > +				break;
> > +			default:
> > +				printf("Unknown format character %c in "
> > +				    "corename `%s'\n", format[i], format);
> Same there.
> 
> > +				return (false);
> > +			}
> > +		}
> > +	}
> > +
> > +	return (true);
> > +}
> > +
> > +static void
> > +corefilename_init(void *arg)
> > +{
> > +	char *format = kern_getenv("kern.corefile");
> Do not use initialization at the local variable declaration.  It is
> a requirement of style(9).
> 

Sorry...

> > +
> > +	if (format != NULL && corefilename_check(format))
> > +		strncpy(corefilename, format, sizeof(corefilename));
> Use of strncpy() is almost always an indication of the bug.  Is it acceptable
> for corefilename be non-null terminated ?
> 

No, it is NOT. Sorry...

> > +}
> > +SYSINIT(corefilename, SI_SUB_KMEM, SI_ORDER_FIRST, corefilename_init, 0);
> > +
> >  static int
> >  sysctl_kern_corefile(SYSCTL_HANDLER_ARGS)
> >  {
> > +	char *format;
> >  	int 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 ||
> > +	    !corefilename_check(format))
> > +		goto out;
> This code somehow misses the check for req->newptr.  Did you verified
> that simply fetching current value for kern.corefile works after the
> patch ?
> 

I'm so sorry, I was too hurried.

I will submit my new patch later.