[RFC/PATCH 1/2] pstore: Protect unlink with read_mutex

[RFC/PATCH 1/2] pstore: Protect unlink with read_mutex

[Namhyung Kim]

When update_ms is set, pstore_get_records() will be called when there's
a new entry.  But unlink can be called at the same time and might
contend with the open-read-close loop.  Depending on the implementation
of platform driver, it may be safe or not.  But I think it'd be better
to protect those race in the first place.

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 fs/pstore/inode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 1781dc50762e..75728dfae0a6 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -197,11 +197,14 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
 	if (err)
 		return err;
 
-	if (p->psi->erase)
+	if (p->psi->erase) {
+		mutex_lock(&p->psi->read_mutex);
 		p->psi->erase(p->type, p->id, p->count,
 			      d_inode(dentry)->i_ctime, p->psi);
-	else
+		mutex_unlock(&p->psi->read_mutex);
+	} else {
 		return -EPERM;
+	}
 
 	return simple_unlink(dir, dentry);
 }
-- 
2.9.3

[RFC/PATCH 2/2] pstore: Convert console write to use ->write_buf

[Namhyung Kim]

Maybe I'm missing something, but I don't know why it needs to copy the
input buffer to psinfo->buf and then write.  Instead we can write the
input buffer directly.  The only implementation that supports console
message (i.e. ramoops) already does it for ftrace messages.

For the upcoming virtio backend driver, it needs to protect psinfo->buf
overwritten from console messages.  If it could use ->write_buf method
instead of ->write, the problem will be solved easily.

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 fs/pstore/platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 14984d902a99..960d66251bab 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -584,8 +584,8 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
 		} else {
 			spin_lock_irqsave(&psinfo->buf_lock, flags);
 		}
-		memcpy(psinfo->buf, s, c);
-		psinfo->write(PSTORE_TYPE_CONSOLE, 0, &id, 0, 0, 0, c, psinfo);
+		psinfo->write_buf(PSTORE_TYPE_CONSOLE, 0, &id, 0,
+				  s, 0, c, psinfo);
 		spin_unlock_irqrestore(&psinfo->buf_lock, flags);
 		s += c;
 		c = e - s;
-- 
2.9.3

Re: [RFC/PATCH 1/2] pstore: Protect unlink with read_mutex

[Kees Cook]

On Tue, Oct 18, 2016 at 6:23 PM, Namhyung Kim <namhyung@kernel.org> wrote:
> When update_ms is set, pstore_get_records() will be called when there's
> a new entry.  But unlink can be called at the same time and might
> contend with the open-read-close loop.  Depending on the implementation
> of platform driver, it may be safe or not.  But I think it'd be better
> to protect those race in the first place.
>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

This looks correct to me, thanks! It should appear in -next shortly...

-Kees

> ---
>  fs/pstore/inode.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 1781dc50762e..75728dfae0a6 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -197,11 +197,14 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
>         if (err)
>                 return err;
>
> -       if (p->psi->erase)
> +       if (p->psi->erase) {
> +               mutex_lock(&p->psi->read_mutex);
>                 p->psi->erase(p->type, p->id, p->count,
>                               d_inode(dentry)->i_ctime, p->psi);
> -       else
> +               mutex_unlock(&p->psi->read_mutex);
> +       } else {
>                 return -EPERM;
> +       }
>
>         return simple_unlink(dir, dentry);
>  }
> --
> 2.9.3
>



-- 
Kees Cook
Nexus Security

Re: [RFC/PATCH 2/2] pstore: Convert console write to use ->write_buf

[Kees Cook]

On Tue, Oct 18, 2016 at 6:23 PM, Namhyung Kim <namhyung@kernel.org> wrote:
> Maybe I'm missing something, but I don't know why it needs to copy the
> input buffer to psinfo->buf and then write.  Instead we can write the
> input buffer directly.  The only implementation that supports console
> message (i.e. ramoops) already does it for ftrace messages.
>
> For the upcoming virtio backend driver, it needs to protect psinfo->buf
> overwritten from console messages.  If it could use ->write_buf method
> instead of ->write, the problem will be solved easily.

Agreed, this looks like needless bounce-buffering. Thanks!

-Kees

>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  fs/pstore/platform.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 14984d902a99..960d66251bab 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -584,8 +584,8 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
>                 } else {
>                         spin_lock_irqsave(&psinfo->buf_lock, flags);
>                 }
> -               memcpy(psinfo->buf, s, c);
> -               psinfo->write(PSTORE_TYPE_CONSOLE, 0, &id, 0, 0, 0, c, psinfo);
> +               psinfo->write_buf(PSTORE_TYPE_CONSOLE, 0, &id, 0,
> +                                 s, 0, c, psinfo);
>                 spin_unlock_irqrestore(&psinfo->buf_lock, flags);
>                 s += c;
>                 c = e - s;
> --
> 2.9.3
>



-- 
Kees Cook
Nexus Security