[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20180618152645.GA15656@bblock-ThinkPad-W530>
Date: Mon, 18 Jun 2018 17:26:45 +0200
From: Benjamin Block <bblock@...ux.vnet.ibm.com>
To: Douglas Gilbert <dgilbert@...erlog.com>
Cc: Al Viro <viro@...IV.linux.org.uk>, Jann Horn <jannh@...gle.com>,
Jens Axboe <axboe@...nel.dk>,
FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>,
"James E.J. Bottomley" <jejb@...ux.vnet.ibm.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>,
linux-block@...r.kernel.org, linux-scsi@...r.kernel.org,
linux-kernel@...r.kernel.org, kernel-hardening@...ts.openwall.com,
security@...nel.org
Subject: Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in
release
On Fri, Jun 15, 2018 at 04:47:47PM -0400, Douglas Gilbert wrote:
> On 2018-06-15 12:40 PM, Al Viro wrote:
> > On Fri, Jun 15, 2018 at 05:23:35PM +0200, Jann Horn wrote:
> >
> >> I've mostly copypasted ib_safe_file_access() over as
> >> scsi_safe_file_access() because I couldn't find a good common header -
> >> please tell me if you know a better way.
> >> The duplicate pr_err_once() calls are so that each of them fires once;
> >> otherwise, this would probably have to be a macro.
> >>
> >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> >> Cc: <stable@...r.kernel.org>
> >> Signed-off-by: Jann Horn <jannh@...gle.com>
> >> ---
> >
> > WTF do you mean, in ->release()? That makes no sense whatsoever -
> > what kind of copy_{to,from}_user() would be possible in there?
>
> The folks responsible are no longer active in kernel development ***
> but as far as I know the async write(command), read(response) were
> added to bsg over 10 years ago as proof-of-concept and never properly
> worked in this async mode. The biggest design problem with it that I'm
> aware of is that two tasks which issue write(commands) at about the
> same time to the same device can receive one another's read(response).
> The tracking of which response belongs to which task is in part the
> reason why the sg driver's data structures are more complex than the
> bsg driver's are. Hence the code work to fix that problem in the bsg
> driver is not trivial and probably a reason why no-one has attempted it.
>
> Once real world users (needing an async SCSI (or general storage)
> pass-through) find out about that bsg "feature", they don't use it.
> They use the sg driver or something else. So the fact that bsg has
> other glaring errors in it in async mode is no surprise to me.
>
> When I took over the maintenance of the sg driver in 1998, it only
> had an async (i.e. write(command), read(response)) interface.
> The SG_IO ioctl was added at the suggestion of Jørg Schilling (of
> cdrecord "fame"). The sg driver implementation was essentially to
> put a write(command) and read(response) back-to-back. The bsg driver
> came along later and started with the synchronous SG_IO ioctl
> interface only. The async write(command)/read(response) functionality
> was added later to bsg. Perhaps that part of the bsg driver should be
> deprecated/withdrawn if a maintainer/rewriter cannot be found.
> [BTW the bsg sync SG_IO ioctl implementation can probably get the
> wrong response, it's just that the window is a lot narrower.]
>
> That said, the bsg driver has lots of other users. For example it is
> the only generic pass-through in Linux for the SAS Management Protocol
> (SMP) used to control SAS based storage enclosures. I have a user space
> package based on it (in Linux) called smp_utils which works well IMO.
> However disk enclosures won't typically have contention between users
> trying to control them and I'm not aware of any disk enclosures that
> support Persistent Reservations. So the bsg driver's "async" problems
> are not a practical issue in this case. Also I believe some high end
> storage hardware uses bsg to communicate with their hardware from their
> user space tools.
>
We definitely use the BSG IOCTL part for FC-Command passthrough (and
other SCSI transports need it for theirs too, SAS, iSCSI, ...). And I am
not aware of any other way to do that right now. So this can not go away
without someone giving us a different way to do that.
That said.. the read()/write() interface is just pointless atm, I don't
see any sane way of using that in userspace.
FWIW, I actually thought about rewriting that part so it becomes
somewhat sane (tracking which thread sends what command and so on), but
haven't had time to really doing it. With the whole discussion now, I am
not sure anyone really needs that anyway.
- Benjamin
--
With Best Regards, Benjamin Block / Linux on IBM Z Kernel Development
IBM Systems & Technology Group / IBM Deutschland Research & Development GmbH
Vorsitz. AufsR.: Martina Koederitz / Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
