If devpts failed to initialize, it would store an ERR_PTR in the global
devpts_mnt. A subsequent open of /dev/ptmx would call devpts_new_index,
which would dereference devpts_mnt and crash.
Avoid storing invalid values in devpts_mnt; leave it NULL instead.
Make both devpts_new_index and devpts_pty_new fail gracefully with
ENODEV in that case, which then becomes the return value to the
userspace open call on /dev/ptmx.
Signed-off-by: Josh Triplett <[email protected]>
---
This fixes a crash found by Fengguang Wu's 0-day service ("BUG: unable to
handle kernel paging request at ffffffee"). It doesn't yet fix the underlying
initialization failure in init_devpts_fs, but it stops that failure from
becoming a kernel crash. I'm working on the initialization failure now.
fs/devpts/inode.c | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index cfe8466..03e9076 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -142,6 +142,8 @@ static inline struct super_block *pts_sb_from_inode(struct inode *inode)
if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
return inode->i_sb;
#endif
+ if (!devpts_mnt)
+ return NULL;
return devpts_mnt->mnt_sb;
}
@@ -525,10 +527,14 @@ static struct file_system_type devpts_fs_type = {
int devpts_new_index(struct inode *ptmx_inode)
{
struct super_block *sb = pts_sb_from_inode(ptmx_inode);
- struct pts_fs_info *fsi = DEVPTS_SB(sb);
+ struct pts_fs_info *fsi;
int index;
int ida_ret;
+ if (!sb)
+ return -ENODEV;
+
+ fsi = DEVPTS_SB(sb);
retry:
if (!ida_pre_get(&fsi->allocated_ptys, GFP_KERNEL))
return -ENOMEM;
@@ -584,11 +590,18 @@ struct inode *devpts_pty_new(struct inode *ptmx_inode, dev_t device, int index,
struct dentry *dentry;
struct super_block *sb = pts_sb_from_inode(ptmx_inode);
struct inode *inode;
- struct dentry *root = sb->s_root;
- struct pts_fs_info *fsi = DEVPTS_SB(sb);
- struct pts_mount_opts *opts = &fsi->mount_opts;
+ struct dentry *root;
+ struct pts_fs_info *fsi;
+ struct pts_mount_opts *opts;
char s[12];
+ if (!sb)
+ return ERR_PTR(-ENODEV);
+
+ root = sb->s_root;
+ fsi = DEVPTS_SB(sb);
+ opts = &fsi->mount_opts;
+
inode = new_inode(sb);
if (!inode)
return ERR_PTR(-ENOMEM);
@@ -676,12 +689,15 @@ static int __init init_devpts_fs(void)
struct ctl_table_header *table;
if (!err) {
+ static struct vfsmount *mnt;
table = register_sysctl_table(pty_root_table);
- devpts_mnt = kern_mount(&devpts_fs_type);
- if (IS_ERR(devpts_mnt)) {
- err = PTR_ERR(devpts_mnt);
+ mnt = kern_mount(&devpts_fs_type);
+ if (IS_ERR(mnt)) {
+ err = PTR_ERR(mnt);
unregister_filesystem(&devpts_fs_type);
unregister_sysctl_table(table);
+ } else {
+ devpts_mnt = mnt;
}
}
return err;
--
2.1.4
On 05/06/2015 08:35 PM, Josh Triplett wrote:
> If devpts failed to initialize, it would store an ERR_PTR in the global
> devpts_mnt. A subsequent open of /dev/ptmx would call devpts_new_index,
> which would dereference devpts_mnt and crash.
>
> Avoid storing invalid values in devpts_mnt; leave it NULL instead.
> Make both devpts_new_index and devpts_pty_new fail gracefully with
> ENODEV in that case, which then becomes the return value to the
> userspace open call on /dev/ptmx.
>
> Signed-off-by: Josh Triplett <[email protected]>
> ---
>
> This fixes a crash found by Fengguang Wu's 0-day service ("BUG: unable to
> handle kernel paging request at ffffffee"). It doesn't yet fix the underlying
> initialization failure in init_devpts_fs, but it stops that failure from
> becoming a kernel crash. I'm working on the initialization failure now.
>
> fs/devpts/inode.c | 30 +++++++++++++++++++++++-------
> 1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index cfe8466..03e9076 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -142,6 +142,8 @@ static inline struct super_block *pts_sb_from_inode(struct inode *inode)
> if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
> return inode->i_sb;
> #endif
> + if (!devpts_mnt)
> + return NULL;
> return devpts_mnt->mnt_sb;
> }
>
> @@ -525,10 +527,14 @@ static struct file_system_type devpts_fs_type = {
> int devpts_new_index(struct inode *ptmx_inode)
> {
> struct super_block *sb = pts_sb_from_inode(ptmx_inode);
> - struct pts_fs_info *fsi = DEVPTS_SB(sb);
> + struct pts_fs_info *fsi;
> int index;
> int ida_ret;
>
> + if (!sb)
> + return -ENODEV;
> +
> + fsi = DEVPTS_SB(sb);
> retry:
> if (!ida_pre_get(&fsi->allocated_ptys, GFP_KERNEL))
> return -ENOMEM;
> @@ -584,11 +590,18 @@ struct inode *devpts_pty_new(struct inode *ptmx_inode, dev_t device, int index,
> struct dentry *dentry;
> struct super_block *sb = pts_sb_from_inode(ptmx_inode);
> struct inode *inode;
> - struct dentry *root = sb->s_root;
> - struct pts_fs_info *fsi = DEVPTS_SB(sb);
> - struct pts_mount_opts *opts = &fsi->mount_opts;
> + struct dentry *root;
> + struct pts_fs_info *fsi;
> + struct pts_mount_opts *opts;
> char s[12];
>
> + if (!sb)
> + return ERR_PTR(-ENODEV);
> +
> + root = sb->s_root;
> + fsi = DEVPTS_SB(sb);
> + opts = &fsi->mount_opts;
> +
> inode = new_inode(sb);
> if (!inode)
> return ERR_PTR(-ENOMEM);
> @@ -676,12 +689,15 @@ static int __init init_devpts_fs(void)
> struct ctl_table_header *table;
>
> if (!err) {
> + static struct vfsmount *mnt;
^^^^^^
Not static storage. Other than that,
Reviewed-by: Peter Hurley <[email protected]>
> table = register_sysctl_table(pty_root_table);
> - devpts_mnt = kern_mount(&devpts_fs_type);
> - if (IS_ERR(devpts_mnt)) {
> - err = PTR_ERR(devpts_mnt);
> + mnt = kern_mount(&devpts_fs_type);
> + if (IS_ERR(mnt)) {
> + err = PTR_ERR(mnt);
> unregister_filesystem(&devpts_fs_type);
> unregister_sysctl_table(table);
> + } else {
> + devpts_mnt = mnt;
> }
> }
> return err;
>
On Thu, May 07, 2015 at 01:52:37PM -0400, Peter Hurley wrote:
> On 05/06/2015 08:35 PM, Josh Triplett wrote:
> > If devpts failed to initialize, it would store an ERR_PTR in the global
> > devpts_mnt. A subsequent open of /dev/ptmx would call devpts_new_index,
> > which would dereference devpts_mnt and crash.
> >
> > Avoid storing invalid values in devpts_mnt; leave it NULL instead.
> > Make both devpts_new_index and devpts_pty_new fail gracefully with
> > ENODEV in that case, which then becomes the return value to the
> > userspace open call on /dev/ptmx.
> >
> > Signed-off-by: Josh Triplett <[email protected]>
> > ---
> >
> > This fixes a crash found by Fengguang Wu's 0-day service ("BUG: unable to
> > handle kernel paging request at ffffffee"). It doesn't yet fix the underlying
> > initialization failure in init_devpts_fs, but it stops that failure from
> > becoming a kernel crash. I'm working on the initialization failure now.
> >
> > fs/devpts/inode.c | 30 +++++++++++++++++++++++-------
> > 1 file changed, 23 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> > index cfe8466..03e9076 100644
> > --- a/fs/devpts/inode.c
> > +++ b/fs/devpts/inode.c
> > @@ -142,6 +142,8 @@ static inline struct super_block *pts_sb_from_inode(struct inode *inode)
> > if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
> > return inode->i_sb;
> > #endif
> > + if (!devpts_mnt)
> > + return NULL;
> > return devpts_mnt->mnt_sb;
> > }
> >
> > @@ -525,10 +527,14 @@ static struct file_system_type devpts_fs_type = {
> > int devpts_new_index(struct inode *ptmx_inode)
> > {
> > struct super_block *sb = pts_sb_from_inode(ptmx_inode);
> > - struct pts_fs_info *fsi = DEVPTS_SB(sb);
> > + struct pts_fs_info *fsi;
> > int index;
> > int ida_ret;
> >
> > + if (!sb)
> > + return -ENODEV;
> > +
> > + fsi = DEVPTS_SB(sb);
> > retry:
> > if (!ida_pre_get(&fsi->allocated_ptys, GFP_KERNEL))
> > return -ENOMEM;
> > @@ -584,11 +590,18 @@ struct inode *devpts_pty_new(struct inode *ptmx_inode, dev_t device, int index,
> > struct dentry *dentry;
> > struct super_block *sb = pts_sb_from_inode(ptmx_inode);
> > struct inode *inode;
> > - struct dentry *root = sb->s_root;
> > - struct pts_fs_info *fsi = DEVPTS_SB(sb);
> > - struct pts_mount_opts *opts = &fsi->mount_opts;
> > + struct dentry *root;
> > + struct pts_fs_info *fsi;
> > + struct pts_mount_opts *opts;
> > char s[12];
> >
> > + if (!sb)
> > + return ERR_PTR(-ENODEV);
> > +
> > + root = sb->s_root;
> > + fsi = DEVPTS_SB(sb);
> > + opts = &fsi->mount_opts;
> > +
> > inode = new_inode(sb);
> > if (!inode)
> > return ERR_PTR(-ENOMEM);
> > @@ -676,12 +689,15 @@ static int __init init_devpts_fs(void)
> > struct ctl_table_header *table;
> >
> > if (!err) {
> > + static struct vfsmount *mnt;
> ^^^^^^
> Not static storage. Other than that,
>
> Reviewed-by: Peter Hurley <[email protected]>
Gah. I deleted that, but apparently didn't "git add". Good catch,
thanks; v2 momentarily.
> > table = register_sysctl_table(pty_root_table);
> > - devpts_mnt = kern_mount(&devpts_fs_type);
> > - if (IS_ERR(devpts_mnt)) {
> > - err = PTR_ERR(devpts_mnt);
> > + mnt = kern_mount(&devpts_fs_type);
> > + if (IS_ERR(mnt)) {
> > + err = PTR_ERR(mnt);
> > unregister_filesystem(&devpts_fs_type);
> > unregister_sysctl_table(table);
> > + } else {
> > + devpts_mnt = mnt;
> > }
> > }
> > return err;
> >
>
If devpts failed to initialize, it would store an ERR_PTR in the global
devpts_mnt. A subsequent open of /dev/ptmx would call devpts_new_index,
which would dereference devpts_mnt and crash.
Avoid storing invalid values in devpts_mnt; leave it NULL instead.
Make both devpts_new_index and devpts_pty_new fail gracefully with
ENODEV in that case, which then becomes the return value to the
userspace open call on /dev/ptmx.
Signed-off-by: Josh Triplett <[email protected]>
Reviewed-by: Peter Hurley <[email protected]>
---
v2: Fix copy-paste error caught by Peter Hurley.
As mentioned in v1, this is separate from fixing the bug that makes devpts fail
to initialize in the first place, but this makes devpts fail gracefully rather
than crashing.
fs/devpts/inode.c | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index cfe8466..016687b 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -142,6 +142,8 @@ static inline struct super_block *pts_sb_from_inode(struct inode *inode)
if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
return inode->i_sb;
#endif
+ if (!devpts_mnt)
+ return NULL;
return devpts_mnt->mnt_sb;
}
@@ -525,10 +527,14 @@ static struct file_system_type devpts_fs_type = {
int devpts_new_index(struct inode *ptmx_inode)
{
struct super_block *sb = pts_sb_from_inode(ptmx_inode);
- struct pts_fs_info *fsi = DEVPTS_SB(sb);
+ struct pts_fs_info *fsi;
int index;
int ida_ret;
+ if (!sb)
+ return -ENODEV;
+
+ fsi = DEVPTS_SB(sb);
retry:
if (!ida_pre_get(&fsi->allocated_ptys, GFP_KERNEL))
return -ENOMEM;
@@ -584,11 +590,18 @@ struct inode *devpts_pty_new(struct inode *ptmx_inode, dev_t device, int index,
struct dentry *dentry;
struct super_block *sb = pts_sb_from_inode(ptmx_inode);
struct inode *inode;
- struct dentry *root = sb->s_root;
- struct pts_fs_info *fsi = DEVPTS_SB(sb);
- struct pts_mount_opts *opts = &fsi->mount_opts;
+ struct dentry *root;
+ struct pts_fs_info *fsi;
+ struct pts_mount_opts *opts;
char s[12];
+ if (!sb)
+ return ERR_PTR(-ENODEV);
+
+ root = sb->s_root;
+ fsi = DEVPTS_SB(sb);
+ opts = &fsi->mount_opts;
+
inode = new_inode(sb);
if (!inode)
return ERR_PTR(-ENOMEM);
@@ -676,12 +689,15 @@ static int __init init_devpts_fs(void)
struct ctl_table_header *table;
if (!err) {
+ struct vfsmount *mnt;
table = register_sysctl_table(pty_root_table);
- devpts_mnt = kern_mount(&devpts_fs_type);
- if (IS_ERR(devpts_mnt)) {
- err = PTR_ERR(devpts_mnt);
+ mnt = kern_mount(&devpts_fs_type);
+ if (IS_ERR(mnt)) {
+ err = PTR_ERR(mnt);
unregister_filesystem(&devpts_fs_type);
unregister_sysctl_table(table);
+ } else {
+ devpts_mnt = mnt;
}
}
return err;
--
2.1.4
On Wed, 6 May 2015 17:35:47 -0700 Josh Triplett <[email protected]> wrote:
> If devpts failed to initialize, it would store an ERR_PTR in the global
> devpts_mnt. A subsequent open of /dev/ptmx would call devpts_new_index,
> which would dereference devpts_mnt and crash.
>
> Avoid storing invalid values in devpts_mnt; leave it NULL instead.
> Make both devpts_new_index and devpts_pty_new fail gracefully with
> ENODEV in that case, which then becomes the return value to the
> userspace open call on /dev/ptmx.
It looks like the system is pretty crippled if init_devptr_fs() fails.
Can the user actually get access to consoles and do useful things in
this situation? Maybe it would be better to just give up and panic?
> @@ -676,12 +689,15 @@ static int __init init_devpts_fs(void)
> struct ctl_table_header *table;
>
> if (!err) {
> + static struct vfsmount *mnt;
static is weird. I assume this was a braino?
> table = register_sysctl_table(pty_root_table);
> - devpts_mnt = kern_mount(&devpts_fs_type);
> - if (IS_ERR(devpts_mnt)) {
> - err = PTR_ERR(devpts_mnt);
> + mnt = kern_mount(&devpts_fs_type);
> + if (IS_ERR(mnt)) {
> + err = PTR_ERR(mnt);
> unregister_filesystem(&devpts_fs_type);
> unregister_sysctl_table(table);
> + } else {
> + devpts_mnt = mnt;
> }
On 05/07/2015 06:59 PM, Andrew Morton wrote:
> On Wed, 6 May 2015 17:35:47 -0700 Josh Triplett <[email protected]> wrote:
>
>> If devpts failed to initialize, it would store an ERR_PTR in the global
>> devpts_mnt. A subsequent open of /dev/ptmx would call devpts_new_index,
>> which would dereference devpts_mnt and crash.
>>
>> Avoid storing invalid values in devpts_mnt; leave it NULL instead.
>> Make both devpts_new_index and devpts_pty_new fail gracefully with
>> ENODEV in that case, which then becomes the return value to the
>> userspace open call on /dev/ptmx.
>
> It looks like the system is pretty crippled if init_devptr_fs() fails.
> Can the user actually get access to consoles and do useful things in
> this situation? Maybe it would be better to just give up and panic?
A single-user console is definitely reachable without devpts.
>From there, one could fixup to a not-broken kernel.
Regards,
Peter Hurley
PS - But I saw you already added these to -mm
On Thu, May 07, 2015 at 03:59:19PM -0700, Andrew Morton wrote:
> On Wed, 6 May 2015 17:35:47 -0700 Josh Triplett <[email protected]> wrote:
>
> > If devpts failed to initialize, it would store an ERR_PTR in the global
> > devpts_mnt. A subsequent open of /dev/ptmx would call devpts_new_index,
> > which would dereference devpts_mnt and crash.
> >
> > Avoid storing invalid values in devpts_mnt; leave it NULL instead.
> > Make both devpts_new_index and devpts_pty_new fail gracefully with
> > ENODEV in that case, which then becomes the return value to the
> > userspace open call on /dev/ptmx.
>
> It looks like the system is pretty crippled if init_devptr_fs() fails.
> Can the user actually get access to consoles and do useful things in
> this situation? Maybe it would be better to just give up and panic?
Mounting devpts doesn't work without it, but you don't *need* to do that
to run a viable system. A full-featured terminal might be unhappy.
init=/bin/sh works, and a console login doesn't strictly require
/dev/pts. A substantial initramfs or rescue system should work without
/dev/pts mounted.
I think this falls under Linus's comments elsewhere about BUG versus
WARN. The system can continue and will function to some degree.
panic() is more suitable for "if I even return from this function,
horrible things will start happening". With this patch, all the
functions provided by devpts gracefully fail if devpts did, so I don't
see a good reason to panic().
> > @@ -676,12 +689,15 @@ static int __init init_devpts_fs(void)
> > struct ctl_table_header *table;
> >
> > if (!err) {
> > + static struct vfsmount *mnt;
>
> static is weird. I assume this was a braino?
Copy/paste issue, yes. Fixed in v2.
- Josh Triplett