Re: [PATCH] sg, bsg: mitigate read/write abuse, block uaccess in release

[Jens Axboe <axboe@kernel.dk>]

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@vger.kernel.org>
>>> Signed-off-by: Jann Horn <jannh@google.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