[PATCH] scsi: sg: Prevent potential double frees in sg driver

[PATCH] scsi: sg: Prevent potential double frees in sg driver

[Nick Desaulniers]

From: Robb Glasser <rglasser@google.com>

sg_ioctl could be spammed by requests, leading to a double free in
__free_pages. This protects the entry points of sg_ioctl where the
memory could be corrupted by a double call to __free_pages if multiple
requests are happening concurrently.

Signed-off-by: Robb Glasser <rglasser@google.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 drivers/scsi/sg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 4fe606b000b4..40fe71f4c863 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -883,8 +883,10 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 			return -ENXIO;
 		if (!access_ok(VERIFY_WRITE, p, SZ_SG_IO_HDR))
 			return -EFAULT;
+		mutex_lock(&sfp->parentdp->open_rel_lock);
 		result = sg_new_write(sfp, filp, p, SZ_SG_IO_HDR,
 				 1, read_only, 1, &srp);
+		mutex_unlock(&sfp->parentdp->open_rel_lock);
 		if (result < 0)
 			return result;
 		result = wait_event_interruptible(sfp->read_wait,
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

Re: [PATCH] scsi: sg: Prevent potential double frees in sg driver

[Greg KH]

On Thu, Aug 03, 2017 at 12:02:47PM -0700, Nick Desaulniers wrote:
> From: Robb Glasser <rglasser@google.com>
> 
> sg_ioctl could be spammed by requests, leading to a double free in
> __free_pages. This protects the entry points of sg_ioctl where the
> memory could be corrupted by a double call to __free_pages if multiple
> requests are happening concurrently.
> 
> Signed-off-by: Robb Glasser <rglasser@google.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  drivers/scsi/sg.c | 2 ++
>  1 file changed, 2 insertions(+)

Why no one on the to: line?

And do you want this in the stable kernel trees?  If so, please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

thanks,

greg k-h

Re: [PATCH] scsi: sg: Prevent potential double frees in sg driver

[Nick Desaulniers]

> Why no one on the to: line?

I usually cc everyone from get_maintainer.pl. Should I be using
--to= then explicitly for named folks, and --cc= for lists?

> And do you want this in the stable kernel trees?

Looks like I can follow up on option #2 once this patch has
been reviewed+merged by maintainers.  I'll note to use
option #1 next time, unless you suggest I send a v2? I can
do so if this patch has a v2+.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] scsi: sg: Prevent potential double frees in sg driver

[Greg KH]

On Thu, Aug 03, 2017 at 12:34:51PM -0700, Nick Desaulniers wrote:
> > Why no one on the to: line?
> 
> I usually cc everyone from get_maintainer.pl. Should I be using
> --to= then explicitly for named folks, and --cc= for lists?

That's usually a good idea, many email clients throw away stuff if there
is nothing on the "To:" line.

> > And do you want this in the stable kernel trees?
> 
> Looks like I can follow up on option #2 once this patch has
> been reviewed+merged by maintainers.  I'll note to use
> option #1 next time, unless you suggest I send a v2? I can
> do so if this patch has a v2+.

If you have to resend it, then add it, otherwise please remember when it
hits Linus's tree to send the git commit id to stable@vger and the
developers there can handle it.

thanks,

greg k-h

Re: [PATCH] scsi: sg: Prevent potential double frees in sg driver

[Christoph Hellwig]

This looks correct and safe to me, but I wonder if anyone relies
on multi-threaded /dev/sg write usage, which would be completely
serialized with this.