| 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
| ||
|
Date: Tue, 15 Oct 2013 11:33:27 -0700
From: Greg KH <greg@...ah.com>
To: Colin Cross <ccross@...gle.com>
Cc: Al Viro <viro@...iv.linux.org.uk>,
Arun KS <arunks.linux@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Matthew Wilcox <matthew@....cx>,
Bruce Fields <bfields@...ldses.org>,
lkml <linux-kernel@...r.kernel.org>,
linux-fsdevel@...r.kernel.org,
vinayak menon <vinayakm.list@...il.com>,
Nagachandra P <nagachandra@...il.com>,
Vikram MP <mp.vikram@...il.com>,
"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v1] seq_file: Fix overflow condition in seq_commit
On Mon, Oct 14, 2013 at 03:57:04PM -0700, Colin Cross wrote:
> On Tue, Aug 20, 2013 at 7:04 AM, Al Viro <viro@...iv.linux.org.uk> wrote:
> > On Tue, Aug 20, 2013 at 08:36:22AM +0100, Al Viro wrote:
> >
> >> Aha. _That_ is a bug, all right - dynamic_dname() is simply not suitable
> >> for that kind of uses. ashmem.c is certainly abusing shmem_file_setup();
> >> feeding that kind of mess as dentry name is a Bad Idea(tm). Anyway,
> >> I'd suggest this for a fix:
> >>
> >> va_list args;
> >> size_t sz;
> >> va_start(args, fmt);
> >> sz = vsnprintf(NULL, 0, fmt, args) + 1;
> >> va_end(args);
> >> if (sz > buflen)
> >> return ERR_PTR(-ENAMETOOLONG);
> >> va_start(args, fmt);
> >> buffer += buflen - sz;
> >> vsprintf(buffer, fmt, args);
> >> va_end(args);
> >> return buffer;
> >>
> >> Either right in dynamic_dname(), or as possibly_fucking_long_dname(),
> >> to be used by shmem.c...
> >
> > Actually, looking at the users of dynamic_dname()... Most of them are
> > fine with dynamic_dname() as it is, with 2 possible exceptions:
> > hugetlb_dname() and shmem_dname(). Both are using "/%s (deleted)",
> > dentry->d_name.name as format... So the solution above may very
> > well be an overkill; something like
> >
> > char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
> > {
> > char *end = buffer + buflen;
> > /* these dentries are never renamed, so d_lock is not needed */
> > if (prepend(&end, &buflen, " (deleted)", 11) ||
> > prepend_name(&end, &buflen, &dentry->d_name) ||
> > prepend(&end, &buflen, "/", 1))
> > end = ERR_PTR(-ENAMETOOLONG);
> > return end;
> > }
> >
> > will do for those two. Care to test the diff below?
> >
> > cope with potentially long ->d_dname() output for shmem/hugetlb
> >
> > dynamic_dname() is both too much and too little for those - the
> > output may be well in excess of 64 bytes dynamic_dname() assumes
> > to be enough (thanks to ashmem feeding really long names to
> > shmem_file_setup()) and vsnprintf() is an overkill for those
> > guys.
> >
> > Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
> > ---
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 87bdb53..83cfb83 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -2724,6 +2724,17 @@ char *dynamic_dname(struct dentry *dentry, char *buffer, int buflen,
> > return memcpy(buffer, temp, sz);
> > }
> >
> > +char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
> > +{
> > + char *end = buffer + buflen;
> > + /* these dentries are never renamed, so d_lock is not needed */
> > + if (prepend(&end, &buflen, " (deleted)", 11) ||
> > + prepend_name(&end, &buflen, &dentry->d_name) ||
> > + prepend(&end, &buflen, "/", 1))
> > + end = ERR_PTR(-ENAMETOOLONG);
> > + return end;
> > +}
> > +
> > /*
> > * Write full pathname from the root of the filesystem into the buffer.
> > */
> > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > index a3f868a..4e5f332 100644
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -916,14 +916,8 @@ static int get_hstate_idx(int page_size_log)
> > return h - hstates;
> > }
> >
> > -static char *hugetlb_dname(struct dentry *dentry, char *buffer, int buflen)
> > -{
> > - return dynamic_dname(dentry, buffer, buflen, "/%s (deleted)",
> > - dentry->d_name.name);
> > -}
> > -
> > static struct dentry_operations anon_ops = {
> > - .d_dname = hugetlb_dname
> > + .d_dname = simple_dname
> > };
> >
> > /*
> > diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> > index b90337c..4a12532 100644
> > --- a/include/linux/dcache.h
> > +++ b/include/linux/dcache.h
> > @@ -336,6 +336,7 @@ extern int d_validate(struct dentry *, struct dentry *);
> > * helper function for dentry_operations.d_dname() members
> > */
> > extern char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
> > +extern char *simple_dname(struct dentry *, char *, int);
> >
> > extern char *__d_path(const struct path *, const struct path *, char *, int);
> > extern char *d_absolute_path(const struct path *, char *, int);
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 8335dbd..e43dc55 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -2909,14 +2909,8 @@ EXPORT_SYMBOL_GPL(shmem_truncate_range);
> >
> > /* common code */
> >
> > -static char *shmem_dname(struct dentry *dentry, char *buffer, int buflen)
> > -{
> > - return dynamic_dname(dentry, buffer, buflen, "/%s (deleted)",
> > - dentry->d_name.name);
> > -}
> > -
> > static struct dentry_operations anon_ops = {
> > - .d_dname = shmem_dname
> > + .d_dname = simple_dname
> > };
> >
> > /**
> > --
> > 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/
>
> Can this patch (committed as 118b23022512eb2f41ce42db70dc0568d00be4ba
> upstream) go in stable? It fixes a regression introduced in 3.9
> dumping /proc/pid/maps for processes with ashmem regions with long
> names.
Looks good to me, now applied to 3.10-stable, thanks.
greg k-h
--
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/
Powered by blists - more mailing lists