[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 18 Jun 2018 09:37:01 -0600
From: Jens Axboe <axboe@...nel.dk>
To: dgilbert@...erlog.com, Al Viro <viro@...IV.linux.org.uk>,
Jann Horn <jannh@...gle.com>
Cc: 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 6/15/18 2:47 PM, 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
It was born with that mode, but I don't think anyone ever really used it.
So it might feasible to simply yank it. That said, just doing a prune
mode at ->release() time doesn't seem like such a hard task.
> 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.]
Feature wise, I don't think it ever changed. The read/write async
mode was included from the get-go.
>
> 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.
>
>
> Just some observations from an interested observer ...
>
> Doug Gilbert
>
>
> *** Well Jens Axbø's Copyright notice is on the bsg driver, together with
> and Peter M. Jones. Since I have been watching the bsg driver I'm
> not aware of any substantial patches or reworks for them. As far as
> I know FUJITA Tomonori did a ground up rewrite of it and he no longer
> works in this area. Makes you wonder what exactly Copyright banners
> mean on some code; 10, 15, 20 years on.
It was never re-written. I handed it over to Fujita about 11 years ago,
but there was never any rewrite.
BTW, don't ever write my name like that, the 'oe' is not a spelled out
ascii variant, it's my name. For Jörg, it's o with umlaut, not the
Danish/Norwegian variant (he's German).
--
Jens Axboe
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.
