Re: [CIFS] [PATCH] consistently use smb_buf_length as be32 for cifs (try 3)

[Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Sun, Mar 20, 2011 at 8:41 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Sat, 19 Mar 2011 22:05:43 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Sat, Mar 19, 2011 at 4:17 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Fri, 18 Mar 2011 14:51:54 -0400
>> > Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
>> >
>> >> On Thu, Mar 17, 2011 at 12:17:24PM -0500, Steve French wrote:
>> >> > If others feel strongly about this, I don't mind changing it as
>> >> > Christoph suggests but
>> >> > - to samba people, "incrementing the rfc1001 length" would be more
>> >> > recognizable (than opencoding the be32_add_cpu macro), and the
>> >> > function name was
>> >> > actually Jeff's suggestion which I liked.
>> >>
>> >> I don't mind the rfc1001 length per se.  What's totally braindead about
>> >> this is having an absolutely trivial wrapper for incrementing a field,
>> >> which has a different name than the field it increments.
>> >>
>> >> If you feel strongly about the rfc1001 length just rename the field.
>> >>
>> >
>> > FWIW, the MS-SMB doc calls this value the "Stream Protocol Length". It
>> > also mentions that this is actually a 24 bit field and the upper 8 bits
>> > are supposed to be zeroed out.
>> >
>> > Should this wrapper check for values that violate that? A little
>> > defensive coding in this area wouldn't hurt.
>>
>> I agree, defensive code wouldn't hurt here. We do actually check IIRC
>> the first byte of it (which acts as an "rfc1001" command code, where
>> zero is what we want for most, in cifs_demultiplex_thread).   Of the
>> next 3 bytes, which serve as a length field, old servers had a small
>> maximum smb size, and thus only read two bytes, eventually some
>> servers allowed you to use just over 64K, and Samba has allowed more
>> than that for years.
>>
>
> cifs_demultiplex_thread checks the first byte of data that is
> *received*. It cannot check that what we're sending is within spec.
> Perhaps this wrapper should.

Yes. agreed, even though the field is initialized to zero in header
assemble and the 4th byte will be zero, and the lengths are checked
earlier it can't hurt to do a final check that the length field did not
overflow.  I will take a look at this.


-- 
Thanks,

Steve