fs: fix access beyond unterminated strings in prints
Subsystems (6)
Amir Goldstein
maintainerPATCH 1/1 fs: fix access beyond unterminated strings in prints
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.
Quoted 4 lines
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
...
Quoted 2 lines
printk+0xa7/0xcf kernel/printk/printk.c:1996 ovl_lookup_index.cold.15+0xe8/0x1f8 fs/overlayfs/namei.c:689
---
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(-)
fs/coda/dir.c +1 -1
@@ -47,7 +47,7 @@ static struct dentry *coda_lookup(struct inode *dir, struct dentry *entry, unsigint 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 ); }
fs/lockd/host.c +1 -1
@@ -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" ));
fs/ocfs2/super.c +1 -1
@@ -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),
fs/overlayfs/namei.c +1 -1
@@ -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);
fs/overlayfs/overlayfs.h +1 -1
@@ -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; }
Signature · 1 line
2.17.1
Jan Harkes
maintainerQuoted 2 lines · Amir Goldstein
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
Miklos Szeredi
maintainerQuoted 17 lines · Amir Goldstein
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.Quoted 4 lines
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...
Quoted 2 lines
printk+0xa7/0xcf kernel/printk/printk.c:1996 ovl_lookup_index.cold.15+0xe8/0x1f8 fs/overlayfs/namei.c:689Reported-by [email protected]Cc Jeff Layton <[email protected]>Cc Jan Harkes <[email protected]>Cc Mark Fasheh <[email protected]>Signed-off-by Amir Goldstein <[email protected]>
Quoted 6 lines
---
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
Quoted 81 lines
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(-)fs/coda/dir.c +1
-1@@ -47,7 +47,7 @@ static struct dentry *coda_lookup(struct inode *dir, struct dentry *entry, unsigint 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 ); }fs/lockd/host.c +1
-1@@ -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" ));fs/ocfs2/super.c +1
-1@@ -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),fs/overlayfs/namei.c +1
-1@@ -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);fs/overlayfs/overlayfs.h +1
-2@@ -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
fs/coda/dir.c static struct dentry *coda_lookup(struct inode *dir, struct dentry *entry, unsig · line 50
Quoted 6 lines · Amir Goldstein
fs/coda/dir.c +1
-1@@ -47,7 +47,7 @@ static struct dentry *coda_lookup(struct inode *dir, struct dentry *entry, unsigint 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