linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] fs: fix access beyond unterminated strings in prints
@ 2018-09-28 18:00 Amir Goldstein
  2018-09-28 19:15 ` Jan Harkes
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Amir Goldstein @ 2018-09-28 18:00 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-unionfs, linux-fsdevel, linux-kernel, Jeff Layton,
	Jan Harkes, Mark Fasheh

KASAN detected slab-out-of-bounds access in printk from overlayfs,
because string format used %*s instead of %.*s.
Found and fixed 4 other places that use %*s incorrectly in filesystems.

> BUG: KASAN: slab-out-of-bounds in string+0x298/0x2d0 lib/vsprintf.c:604
> Read of size 1 at addr ffff8801c36c66ba by task syz-executor2/27811
>
> CPU: 0 PID: 27811 Comm: syz-executor2 Not tainted 4.19.0-rc5+ #36
...
>  printk+0xa7/0xcf kernel/printk/printk.c:1996
>  ovl_lookup_index.cold.15+0xe8/0x1f8 fs/overlayfs/namei.c:689

Reported-by: syzbot+376cea2b0ef340db3dd4@syzkaller.appspotmail.com
Cc: Jeff Layton <jlayton@kernel.org>
Cc: Jan Harkes <jaharkes@cs.cmu.edu>
Cc: Mark Fasheh <mark@fasheh.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Miklos,

I chose not to split the patches per fs in the hope that maintainers
would quickly ack the patch and ask you to carry it for them.

If this doesn't happen, feel free to drop non-acked bits from the patch.

Thanks,
Amir.

 fs/coda/dir.c            | 2 +-
 fs/lockd/host.c          | 2 +-
 fs/ocfs2/super.c         | 2 +-
 fs/overlayfs/namei.c     | 2 +-
 fs/overlayfs/overlayfs.h | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/coda/dir.c b/fs/coda/dir.c
index 00876ddadb43..23ee5de8b4be 100644
--- a/fs/coda/dir.c
+++ b/fs/coda/dir.c
@@ -47,7 +47,7 @@ static struct dentry *coda_lookup(struct inode *dir, struct dentry *entry, unsig
 	int type = 0;
 
 	if (length > CODA_MAXNAMLEN) {
-		pr_err("name too long: lookup, %s (%*s)\n",
+		pr_err("name too long: lookup, %s (%.*s)\n",
 		       coda_i2s(dir), (int)length, name);
 		return ERR_PTR(-ENAMETOOLONG);
 	}
diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index d35cd6be0675..93fb7cf0b92b 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -341,7 +341,7 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
 	};
 	struct lockd_net *ln = net_generic(net, lockd_net_id);
 
-	dprintk("lockd: %s(host='%*s', vers=%u, proto=%s)\n", __func__,
+	dprintk("lockd: %s(host='%.*s', vers=%u, proto=%s)\n", __func__,
 			(int)hostname_len, hostname, rqstp->rq_vers,
 			(rqstp->rq_prot == IPPROTO_UDP ? "udp" : "tcp"));
 
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 3415e0b09398..b74435dc85fd 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -259,7 +259,7 @@ static int ocfs2_osb_dump(struct ocfs2_super *osb, char *buf, int len)
 
 	if (cconn) {
 		out += snprintf(buf + out, len - out,
-				"%10s => Stack: %s  Name: %*s  "
+				"%10s => Stack: %s  Name: %.*s  "
 				"Version: %d.%d\n", "Cluster",
 				(*osb->osb_cluster_stack == '\0' ?
 				 "o2cb" : osb->osb_cluster_stack),
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index f28711846dd6..9c0ca6a7becf 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -686,7 +686,7 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
 			index = NULL;
 			goto out;
 		}
-		pr_warn_ratelimited("overlayfs: failed inode index lookup (ino=%lu, key=%*s, err=%i);\n"
+		pr_warn_ratelimited("overlayfs: failed inode index lookup (ino=%lu, key=%.*s, err=%i);\n"
 				    "overlayfs: mount with '-o index=off' to disable inodes index.\n",
 				    d_inode(origin)->i_ino, name.len, name.name,
 				    err);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index f61839e1054c..c096f12657cd 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -152,7 +152,7 @@ static inline int ovl_do_setxattr(struct dentry *dentry, const char *name,
 				  const void *value, size_t size, int flags)
 {
 	int err = vfs_setxattr(dentry, name, value, size, flags);
-	pr_debug("setxattr(%pd2, \"%s\", \"%*s\", 0x%x) = %i\n",
+	pr_debug("setxattr(%pd2, \"%s\", \"%.*s\", 0x%x) = %i\n",
 		 dentry, name, (int) size, (char *) value, flags, err);
 	return err;
 }
-- 
2.17.1

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] fs: fix access beyond unterminated strings in prints
  2018-09-28 18:00 [PATCH] fs: fix access beyond unterminated strings in prints Amir Goldstein
@ 2018-09-28 19:15 ` Jan Harkes
  2018-10-02  9:26 ` Miklos Szeredi
  2018-10-04 15:05 ` David Howells
  2 siblings, 0 replies; 4+ messages in thread
From: Jan Harkes @ 2018-09-28 19:15 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Miklos Szeredi, linux-unionfs, linux-fsdevel, linux-kernel,
	Jeff Layton, Mark Fasheh

On Fri, Sep 28, 2018 at 09:00:48PM +0300, Amir Goldstein wrote:
> I chose not to split the patches per fs in the hope that maintainers
> would quickly ack the patch and ask you to carry it for them.

Ack for Coda. I actually have a patch queued somewhere that dropped
printing the name and only prints the length, but this works too.

Jan

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] fs: fix access beyond unterminated strings in prints
  2018-09-28 18:00 [PATCH] fs: fix access beyond unterminated strings in prints Amir Goldstein
  2018-09-28 19:15 ` Jan Harkes
@ 2018-10-02  9:26 ` Miklos Szeredi
  2018-10-04 15:05 ` David Howells
  2 siblings, 0 replies; 4+ messages in thread
From: Miklos Szeredi @ 2018-10-02  9:26 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: overlayfs, linux-fsdevel, linux-kernel, Jeff Layton, Jan Harkes,
	Mark Fasheh

On Fri, Sep 28, 2018 at 8:00 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> KASAN detected slab-out-of-bounds access in printk from overlayfs,
> because string format used %*s instead of %.*s.
> Found and fixed 4 other places that use %*s incorrectly in filesystems.
>
>> BUG: KASAN: slab-out-of-bounds in string+0x298/0x2d0 lib/vsprintf.c:604
>> Read of size 1 at addr ffff8801c36c66ba by task syz-executor2/27811
>>
>> CPU: 0 PID: 27811 Comm: syz-executor2 Not tainted 4.19.0-rc5+ #36
> ...
>>  printk+0xa7/0xcf kernel/printk/printk.c:1996
>>  ovl_lookup_index.cold.15+0xe8/0x1f8 fs/overlayfs/namei.c:689
>
> Reported-by: syzbot+376cea2b0ef340db3dd4@syzkaller.appspotmail.com
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: Jan Harkes <jaharkes@cs.cmu.edu>
> Cc: Mark Fasheh <mark@fasheh.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Miklos,
>
> I chose not to split the patches per fs in the hope that maintainers
> would quickly ack the patch and ask you to carry it for them.

It's not as simple. While each one looks like a typo, not all of them
may be bugs, and quite likely introduced in different versions, etc...

E.g. look at the fuse_do_setxattr() one: it's there since the initial
merge of overlayfs, but back then it wasn't a bug, since it was only
called for setting the OPAQUE attribute and the supplied value was a
string constant, so the printk would work despite the typo.  Then it
grew users where the string was neither null terminated nor printable
(file handle).  A proper fix would be to print a hex dump of the value
instead, or don't print the value at all...

I'll split and fix the overlay ones, but can't vouch for the others.

Thanks,
Miklos

>
> If this doesn't happen, feel free to drop non-acked bits from the patch.
>
> Thanks,
> Amir.
>
>  fs/coda/dir.c            | 2 +-
>  fs/lockd/host.c          | 2 +-
>  fs/ocfs2/super.c         | 2 +-
>  fs/overlayfs/namei.c     | 2 +-
>  fs/overlayfs/overlayfs.h | 2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/coda/dir.c b/fs/coda/dir.c
> index 00876ddadb43..23ee5de8b4be 100644
> --- a/fs/coda/dir.c
> +++ b/fs/coda/dir.c
> @@ -47,7 +47,7 @@ static struct dentry *coda_lookup(struct inode *dir, struct dentry *entry, unsig
>         int type = 0;
>
>         if (length > CODA_MAXNAMLEN) {
> -               pr_err("name too long: lookup, %s (%*s)\n",
> +               pr_err("name too long: lookup, %s (%.*s)\n",
>                        coda_i2s(dir), (int)length, name);
>                 return ERR_PTR(-ENAMETOOLONG);
>         }
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index d35cd6be0675..93fb7cf0b92b 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -341,7 +341,7 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
>         };
>         struct lockd_net *ln = net_generic(net, lockd_net_id);
>
> -       dprintk("lockd: %s(host='%*s', vers=%u, proto=%s)\n", __func__,
> +       dprintk("lockd: %s(host='%.*s', vers=%u, proto=%s)\n", __func__,
>                         (int)hostname_len, hostname, rqstp->rq_vers,
>                         (rqstp->rq_prot == IPPROTO_UDP ? "udp" : "tcp"));
>
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index 3415e0b09398..b74435dc85fd 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -259,7 +259,7 @@ static int ocfs2_osb_dump(struct ocfs2_super *osb, char *buf, int len)
>
>         if (cconn) {
>                 out += snprintf(buf + out, len - out,
> -                               "%10s => Stack: %s  Name: %*s  "
> +                               "%10s => Stack: %s  Name: %.*s  "
>                                 "Version: %d.%d\n", "Cluster",
>                                 (*osb->osb_cluster_stack == '\0' ?
>                                  "o2cb" : osb->osb_cluster_stack),
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index f28711846dd6..9c0ca6a7becf 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -686,7 +686,7 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
>                         index = NULL;
>                         goto out;
>                 }
> -               pr_warn_ratelimited("overlayfs: failed inode index lookup (ino=%lu, key=%*s, err=%i);\n"
> +               pr_warn_ratelimited("overlayfs: failed inode index lookup (ino=%lu, key=%.*s, err=%i);\n"
>                                     "overlayfs: mount with '-o index=off' to disable inodes index.\n",
>                                     d_inode(origin)->i_ino, name.len, name.name,
>                                     err);
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index f61839e1054c..c096f12657cd 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -152,7 +152,7 @@ static inline int ovl_do_setxattr(struct dentry *dentry, const char *name,
>                                   const void *value, size_t size, int flags)
>  {
>         int err = vfs_setxattr(dentry, name, value, size, flags);
> -       pr_debug("setxattr(%pd2, \"%s\", \"%*s\", 0x%x) = %i\n",
> +       pr_debug("setxattr(%pd2, \"%s\", \"%.*s\", 0x%x) = %i\n",
>                  dentry, name, (int) size, (char *) value, flags, err);
>         return err;
>  }
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] fs: fix access beyond unterminated strings in prints
  2018-09-28 18:00 [PATCH] fs: fix access beyond unterminated strings in prints Amir Goldstein
  2018-09-28 19:15 ` Jan Harkes
  2018-10-02  9:26 ` Miklos Szeredi
@ 2018-10-04 15:05 ` David Howells
  2 siblings, 0 replies; 4+ messages in thread
From: David Howells @ 2018-10-04 15:05 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: dhowells, Miklos Szeredi, linux-unionfs, linux-fsdevel,
	linux-kernel, Jeff Layton, Jan Harkes, Mark Fasheh

Amir Goldstein <amir73il@gmail.com> wrote:

> @@ -47,7 +47,7 @@ static struct dentry *coda_lookup(struct inode *dir, struct dentry *entry, unsig
>  	int type = 0;
>  
>  	if (length > CODA_MAXNAMLEN) {
> -		pr_err("name too long: lookup, %s (%*s)\n",
> +		pr_err("name too long: lookup, %s (%.*s)\n",

Use %pd instead?

David

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-10-04 15:05 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-28 18:00 [PATCH] fs: fix access beyond unterminated strings in prints Amir Goldstein
2018-09-28 19:15 ` Jan Harkes
2018-10-02  9:26 ` Miklos Szeredi
2018-10-04 15:05 ` David Howells

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).