Mailing List Archive

Steve French wrote:
> You are correct that the CIFS code calls SendReceive in cases in which
> the buffer may be too small to fit a large SMB response, and that
> should be fixed (e.g. to avoid possible overflows due to a server
> bug), None of the eight cases (SMB TreeDisconnect, SMB uLogoff, SMB
> Close, SMB FindClose etc.) in which a small buffer is passed in to
> SendReceive return more than a few dozen bytes (and they are fixed
> size responses), but I agree that we have to be safe (and we have seen
> at least one server corrupt the bcc in the ulogoffX response and
> another on the NTCreateX response) so it would be good to fix.
>
Well, mounting shares from untrusted server is quite uncommon, still
buffer overrun shall be considered a serious issue, imho. If the buffer
was on the stack, that would be directly exploitable.
> There are probably better ways to handle this though than passing in
> the buffer size as your patch does. Since there are only two buffer
> sizes that CIFS uses - it would be easier to pass in (or out) a flag
> which indicates the buffer size. But the function SendReceive2
> already does that - and the easier way to handle this seems to be
> changing the eight places in fs/cifs/cifssmb.c which call
> small_smb_init and then call SendReceive, to call SendReceive2
> instead.
>
That is mostly a matter of taste. SendReceive takes buffer pointer and
SendReceive2 takes kvec - I'd rather prefer to stay with the same
function used for both small and large buffers, using two different
functions just because 2 different buffer sizes are passed around is
ugly, imho, nonorthogonal at least. I guess that initial intention was
to use SendReceive2 in places where kvec is really needed. Also these
functions are almost identical, maybe SendReceive should wrap
SendReceive2 preparing kvec with a buffer pointer passed to it?

Obviously it is up to you, as a maintainer. I'd prefer adding a small
header to each buffer with the buffer size and perhaps a type, or even a
destructor function pointer. Simple macros could be used to obtain
buffer size, given the buffer body pointer, or to dispose the buffer.
That would save from checking the buffer type all over the code
explicitly, or even worse, make strange assumptions about the type of
buffer being passed - as we can see this is error-prone. That for a
little cost of a few additional bytes per buffer.


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, Nov 09, 2007 at 11:59:46AM +0100, Przemyslaw Wegrzyn wrote:
> Steve French wrote:
> > You are correct that the CIFS code calls SendReceive in cases in which
> > the buffer may be too small to fit a large SMB response, and that
> > should be fixed (e.g. to avoid possible overflows due to a server
> > bug), None of the eight cases (SMB TreeDisconnect, SMB uLogoff, SMB
> > Close, SMB FindClose etc.) in which a small buffer is passed in to
> > SendReceive return more than a few dozen bytes (and they are fixed
> > size responses), but I agree that we have to be safe (and we have seen
> > at least one server corrupt the bcc in the ulogoffX response and
> > another on the NTCreateX response) so it would be good to fix.
> >
> Well, mounting shares from untrusted server is quite uncommon, still
> buffer overrun shall be considered a serious issue, imho.

Also, a compromised machine on the same network could forge the
malicious reply in some cases, right?

--b.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

I have done an analysis of the SMB functions (56 callers of
SendReceive, 4 of SendReceive2 and 2 callers of
SendReceiveBlockingLock) and found additional changes which should
help performance, by reducing the number of expensive large buffer
allocations and also by freeing buffers back to the pool faster. See
below. The obvious need is to create an SendReceive-NoResponse (or
equivalent) which
frees the SMB request buffer after send, and does not copy into an smb
response buffer. The following functions need to be changed to use
that (and also address the problem that Przemyslaw noted):

TreeDisconnect, uLogoff, Close, findClose, SetFileSize, SetFileTimes,
(Lock and PosixLock need slightly more complicated change
since only some of their paths can use proposed new function)

There is a longer list of functions which should be optimized to call
a SendReceiveNoResponse like function as well (but we can do this
later after 2.6.24) since we also do not parse their response SMB (it
would make buffer allocation more efficient to free the request
buffers faster and avoid a memcpy):

PosixDelFile, DelFile, Rmdir, MkDir, Rename, RenameOpenFile, Copy,
CreateSymLink, CreateHardLinkUnix, CreateHardLink, SetPosixACL,
SetFSUnixInfo (also change to small buf allocate), SetEOF, SetTimes,
SetAttrLegacy, UnixSetPerms, SetEA

Some other functions can be changed to small buffer alloc requests:
Negotiate, SetFSUnixInfo, QueryReparseLink, GetExtAttr, the 6 QFSInfos
requests, Notify


Explanatory Key for the list below:
-------------------------------------------------
SR = the function currently uses SendReceive
SR2 = uses SendReceive2()
SRB = uses SendReceiveBlockingLock
large = uses smb_init (allocates large buffer)
small = uses small_smb_init (allocates small buffer)

LR = getting a large response back from the server is common
NR = no return SMB needed to be parsed by caller, processing only
depends on value of SMB error
ID = idempotent (does not change state significantly if repeated)
L = can be long operation (and take longer than a few seconds)
B = operation may block indefinitely
PB = operation has variable length request size due to path name in request
IOV = needs to send iovec

List of SMB Functions and their characteristics
-------------------------------------------------------------
(in fs/cifs/cifssmb.c)
Negotiate large LR*, ID
TreeDisconnect small SR NR
uLogoff small SR NR
PosixDelFile large SR PB,NR
DelFile large SR PB,NR
RmDir large SR PB,NR
MkDir large SR PB,NR
PosixCreate large SR PB
LegacyOpen large SR PB
Open large SR PB
Read small SR2 LR, ID
Write large SR (need mixed user/kernel IOV to change) L, ID?
Write2 small SR2 IOV, L
Lock small SR/SRB NR, B
PosixLock small SR/SRB (NR on setpath), B
Close small SR NR
Rename large SR NR, PB
RenameOpenFile large SR NR, PB
Copy large SR NR, PB
CreateSymLink large SR NR, PB
CreateHardLinkU large SR NR, PB
CreateHardLink large SR NR, PB
QuerySymLink large SR LR, PB, ID
QueryReparseLk large SR LR, ID
QueryPosixACL large SR LR, PB, ID
SetPosixACL large SR NR, PB
GetExtAttr large SR LR, ID
GetCIFSACL small SR2 LR, ID
QueryInformatn large SR PB, ID
QPathInfo large SR PB, ID
UnixQPathInfo large SR PB, ID
FindSingle large SR PB, LR?, ID (remove)
FindFirst large SR PB, LR
FindNext large SR PB*, LR, ID* (resume key)
FindClose small SR NR
GetSrvInodeNum large SR PB, ID
GetDFSRefer large SR PB, LR, ID
OldQFSInfo large SR ID
QFSInfo large SR ID
QFSAttribute large SR ID
QFSDeviceInfo large SR ID
QFSUnixInfo large SR ID
SetFSUnixInfo large SR ID, NR
QFSPosixInfo large SR ID
SetEOF large SR PB, NR
SetFileSize small SR NR
SetFileTimes small SR NR
SetTimes large SR PB, NR
SetAttrLegacy large SR PB, NR
UnixSetPerms large SR PB, NR
Notify large SR
QueryAllEAs large SR PB, LR, ID
QueryEA large SR PB, LR, ID
SetEA large SR PB, NR

(in fs/cifs/sess.c)
SessionSetup large SR2 IOV, large request, LR

(in fs/cifs/connect.c)
TreeConnect large SR PB
other uses of SendReceive* in this file are obsolete

(in fs/cifs/transport.c)
send_lock_cancel no buffer allocate, SR NR function looks broken



On Nov 9, 2007 4:59 AM, Przemyslaw Wegrzyn <czajnik@czajsoft.pl> wrote:
> to use SendReceive2 in places where kvec is really needed. Also these
> functions are almost identical, maybe SendReceive should wrap
> SendReceive2 preparing kvec with a buffer pointer passed to it?
>
> Obviously it is up to you, as a maintainer. I'd prefer adding a small
> header to each buffer with the buffer size and perhaps a type, or even a
> destructor function pointer. Simple macros could be used to obtain
> buffer size, given the buffer body pointer, or to dispose the buffer.
> That would save from checking the buffer type all over the code
> explicitly, or even worse, make strange assumptions about the type of
> buffer being passed - as we can see this is error-prone. That for a
> little cost of a few additional bytes per buffer.
That might be better, although without memory pools, this would perform
much worse

>



--
Thanks,

Steve
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Steve French wrote:
> below. The obvious need is to create an SendReceive-NoResponse (or
> equivalent) which
> frees the SMB request buffer after send, and does not copy into an smb
> response buffer. The following functions need to be changed to use
>
How about modifying SendReceive to behave like that if NULL is passed as
output buffer ?

>> Obviously it is up to you, as a maintainer. I'd prefer adding a small
>> header to each buffer with the buffer size and perhaps a type, or even a
>> destructor function pointer. Simple macros could be used to obtain
>> buffer size, given the buffer body pointer, or to dispose the buffer.
>> That would save from checking the buffer type all over the code
>> explicitly, or even worse, make strange assumptions about the type of
>> buffer being passed - as we can see this is error-prone. That for a
>> little cost of a few additional bytes per buffer.
>>
> That might be better, although without memory pools, this would perform
> much worse
>
Why ? I don't get your point here.

Przemyslaw

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Nov 10, 2007 7:03 AM, Przemyslaw Wegrzyn <czajnik@czajsoft.pl> wrote:
> Steve French wrote:
> > That might be better, although without memory pools, this would perform
> > much worse
> >
> Why ? I don't get your point here.
>
> Przemyslaw
>

What I meant is that two fixed size memory pools rather variable size
kmallocs helps performance. By using two fixed size buffers (small -
which fits the typical smb request and response, and large which fits
the maximum size request other than write which is handled via an
iovec) and taking advantage of memory pools, cifs can always make
progress even in low memory situations (reducing the likelihood of
deadlock) and it reduces the number of times that cifs has to do the
very expensive allocation of 16.5K. If cifs only used variable size
request and response buffers, since they are frequently more than one
page in size we would be forcing the memory manager to find contiguous
pages which can be very slow.



--
Thanks,

Steve
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Steve French wrote:
> On Nov 10, 2007 7:03 AM, Przemyslaw Wegrzyn <czajnik@czajsoft.pl> wrote:
>
>> Steve French wrote:
>>
>>> That might be better, although without memory pools, this would perform
>>> much worse
>>>
>>>
>> Why ? I don't get your point here.
>>
> What I meant is that two fixed size memory pools rather variable size
> kmallocs helps performance. By using two fixed size buffers (small -
> which fits the typical smb request and response, and large which fits
> the maximum size request other than write which is handled via an
> iovec) and taking advantage of memory pools, cifs can always make
> progress even in low memory situations (reducing the likelihood of
> deadlock) and it reduces the number of times that cifs has to do the
> very expensive allocation of 16.5K. If cifs only used variable size
> request and response buffers, since they are frequently more than one
> page in size we would be forcing the memory manager to find contiguous
> pages which can be very slow.
>
That was not really my intention to allocate variable size buffers -
it's perfectly Ok for me to have 2 fixed-size pools. What I meant was to
allocate buffers with some kind of header containing buffer size and
type of buffer or whatever needed info. Obviously, you could use
pointers to buffer body all over the code, functions/macros that need to
access the header would move the pointer back by sizeof(header). Then
one could e.g. provide a single function for disposing the buffer, and
no longer care which pool is it allocated from, for the additional
memory cost - the function would look into the header to check that for
you. Hope it's clear now (my English is far from perfect).

Przemyslaw

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/