linux-kernel - Re: [PATCH] Fix proc_file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20090807151657.d577dd2d.akpm@linux-foundation.org>
Date:	Fri, 7 Aug 2009 15:16:57 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Stefani Seibold <stefani@...bold.net>
Cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] Fix proc_file_write missing ppos update

On Fri, 07 Aug 2009 23:43:07 +0200
Stefani Seibold <stefani@...bold.net> wrote:

> > > +			*ppos += rv;
> > >  	}
> > >  	return rv;
> > >  }
> > 
> > Yes, that's odd.
> > 
> > I worry that there might be procfs write handlers which are looking at
> > *ppos and whose behaviour might be altered by this patch.
> > 
> > <searches a bit>
> > 
> > Look at arch/s390/appldata/appldata_base.c:appldata_timer_handler().
> > 
> > static int
> > appldata_timer_handler(ctl_table *ctl, int write, struct file *filp,
> > 			   void __user *buffer, size_t *lenp, loff_t *ppos)
> > {
> > 	int len;
> > 	char buf[2];
> > 
> > 	if (!*lenp || *ppos) {
> > 		*lenp = 0;
> > 		return 0;
> > 	}
> > 
> > 
> 
> This function will be handled IMHO by the proc_sys_call_handler which
> has nothing to do with the proc_file_write.
> /proc/sys/... file access should be not touched because there are
> handled differently. 

hm, OK, fail.

> > Prior to your change, an application which opened that proc file and
> > repeatedly wrote to the fd would repeatedly start and stop the timer.
> > 
> > After your change, the second and successive writes would have no
> > effect unless the application was changed to lseek back to the start of
> > the "file".
> > 
> > And that was just the second file I looked at via
> > 
> > 	$EDITOR $(grep -l '[*]ppos' $(grep -rl _proc_ .))
> 
> Yes, i think you are right, i have forseen also that there maybe some
> pitfalls. The question is: is there any appplication which will be
> broken by this patch?

There is no way of telling.  We have to assume that there will be such
code out there.

> So what is your suggestion? Should we drop this patch or should we
> analyze the users and fix it?

Well.

We could review all implementations of ->write_proc.  There only seem
to be twenty or so.

If any of them will have their behaviour altered by this patch then
let's look at those on a case-by-case basis and decide whether making
this change will have an acceptable risk.

If we _do_ find one for which we simply cannot make this behavioural
change then..  ugh.  We could perhaps add a new `bool
proc_dir_entry.implement_old_broken_behaviour' and set that flag for
the offending driver(s) and test it within proc_write_file().

Or we could do

	if (pde->write_proc_new) {
		rv = pde->write_proc_new(file, buffer, count, pde->data);
		*ppos += rv;
	} else {
		rv = pde->write_proc(file, buffer, count, pde->data);
	}

which is really the same thing and isn't obviously better ;)

> My opinion is to fix it, because it is wrong and it limits the usage of
> the proc_write operation. Many embedded developers like me count on proc
> support, because it is much simpler to use than the seqfile thing.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/