Bug 6384 - samba allows clients to create symlinks that are longer than it will read
Summary: samba allows clients to create symlinks that are longer than it will read
Status: RESOLVED FIXED
Alias: None
Product: CifsVFS
Classification: Unclassified
Component: kernel fs (show other bugs)
Version: 2.6
Hardware: Other Linux
: P3 normal
Target Milestone: ---
Assignee: Jeff Layton
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-21 10:01 UTC by Jeff Layton
Modified: 2009-05-28 14:01 UTC (History)
0 users

See Also:


Attachments
capture showing error (7.34 KB, application/octet-stream)
2009-05-21 10:02 UTC, Jeff Layton
no flags Details
Evil hack to test :-) (491 bytes, patch)
2009-05-21 15:40 UTC, Jeremy Allison
no flags Details
output from smbd -d 10 -i (120.49 KB, text/plain)
2009-05-21 18:37 UTC, Jeff Layton
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff Layton 2009-05-21 10:01:16 UTC
I've been testing some CIFS patches with fsstress. As part of its testing, that program will create some really long symlinks. Here's one (stat'ed via local fs):

# stat /export/fsstress/p7/d5/d1a/d2a/l64
  File: `/export/fsstress/p7/d5/d1a/d2a/l64' -> `xxxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx/xxxxxxxxx'
  Size: 2360      	Blocks: 8          IO Block: 4096   symbolic link
Device: fd00h/64768d	Inode: 188384      Links: 1
Access: (0777/lrwxrwxrwx)  Uid: (   99/  nobody)   Gid: (   99/  nobody)
Access: 2009-05-21 10:28:16.556587266 -0400
Modify: 2009-05-21 10:27:59.915631842 -0400
Change: 2009-05-21 10:28:15.143597074 -0400

...when I stat this via CIFS, I see this:

# stat /mnt/cifs/fsstress/p7/d5/d1a/d2a/l64
  File: `/mnt/cifs/fsstress/p7/d5/d1a/d2a/l64'stat: cannot read symbolic link `/mnt/cifs/fsstress/p7/d5/d1a/d2a/l64': Input/output error

  Size: 2360      	Blocks: 8          IO Block: 16384  symbolic link
Device: 13h/19d	Inode: 188384      Links: 1
Access: (0777/lrwxrwxrwx)  Uid: (   99/  nobody)   Gid: (   99/  nobody)
Access: 2009-05-21 10:28:16.556587200 -0400
Modify: 2009-05-21 10:27:59.915631800 -0400
Change: 2009-05-21 10:28:15.143597000 -0400

...on the wire, I see that samba is returning this error:

NT Status: STATUS_BUFFER_OVERFLOW (0x80000005)

It doesn't seem like samba ought to let you create symlinks that are bigger than it allows you to read.
Comment 1 Jeff Layton 2009-05-21 10:02:06 UTC
Created attachment 4173 [details]
capture showing error

Capture showing samba returning error for the above symlink.
Comment 2 Jeremy Allison 2009-05-21 15:34:12 UTC
Jeff, can you get me a debug level 10 log of this error ? We just call down to the underlying system symlink() call here, and return whatever error we get.
On symlink read we call readlink() with a buffer of size PATH_MAX. I'm guessing that the size of PATH_MAX on compilation is too small. If PATH_MAX isn't defined on a system we default to 1024.

Inside /usr/include/linux/limits.h, we have: #define PATH_MAX        4096

I'm wondering if that is correctly being pulled in. Looks like a configure error to me rather than a logic error in the code.

Jeremy.
Comment 3 Jeremy Allison 2009-05-21 15:40:57 UTC
Created attachment 4174 [details]
Evil hack to test :-)

Ok, apply this then run smbd -i and it'll print PATH_MAX for you :-).
Just to see what we have internally....
Jeremy.
Comment 4 Jeff Layton 2009-05-21 18:37:02 UTC
Created attachment 4175 [details]
output from smbd -d 10 -i

Log from smbd -d 10 -i. I created a large symlink via cifs mount and then tried to stat it. The stat returned -EIO (due to the error returned by samba).

If you like I'll still try to do the check for PATH_MAX. I think though that this sort of rules that out as the problem:

fill_share_mode_lock failed
call_trans2qfilepathinfo testlink (fnum = -1) level=513 call=5 total_data=0
dos_mode: testlink
dos_mode_from_sbuf returning a
dos_mode returning a
call_trans2qfilepathinfo: SMB_QUERY_FILE_UNIX_LINK
send_trans2_replies: max_data_bytes 4000 exceeded by data 4722
t2_rep: params_sent_thistime = 2, data_sent_thistime = 4000, useable_space = 16410
t2_rep: params_to_send = 2, data_to_send = 4000, paramsize = 2, datasize = 4000
error packet at smbd/trans2.c(848) cmd=50 (SMBtrans2) STATUS_BUFFER_OVERFLOW
size=4060
smb_com=0x32
smb_rcls=5

Not sure where the 4000 byte limit is coming from, but even if it were 4096, we'd still be exceeding it with a data size of 4722 (assuming I'm reading this output correctly).
Comment 5 Jeremy Allison 2009-05-21 19:32:55 UTC
Ah ok, this is a client bug then. Note this:

send_trans2_replies: max_data_bytes 4000 exceeded by data 4722

The max_data_bytes is a uint16_t value sent by the client in the initial trans2 call. We read it from the incoming client packet here (smbd/trans2.c: 7639 in the 3.2.x git tree):

        state->max_data_return  = SVAL(req->inbuf, smb_mdrcnt);

smb_mdrcnt is defined as smb_vwv3 in the words area of the SMB. My guess is the client is only allocating one kernel page for this return value. You'll need to up that if you want to get PATH_MAX (4096) bytes back :-).

Jeremy.
Comment 6 Jeremy Allison 2009-05-21 19:42:53 UTC
Oh yes, here it is inside cifssmb.c: CIFSSMBUnixQuerySymLink()
we have:

        /* BB find exact max data count below from sess structure BB */
        pSMB->MaxDataCount = cpu_to_le16(4000);

You'll need to up that for all possible calls that can return pathnames.

Jeremy.
Comment 7 Jeremy Allison 2009-05-21 19:44:11 UTC
Ball's back in your court, I think :-).

Jeremy.
Comment 8 Jeff Layton 2009-05-21 19:48:41 UTC
Ahh, good catch. I'll see about fixing this -- thanks for the help!
Comment 9 Steve French 2009-05-28 08:40:31 UTC
Fix is in cifs-2.6.git now
Comment 10 Jeff Layton 2009-05-28 12:05:45 UTC
Closing bug. Thanks for the help Jeremy...
Comment 11 Jeremy Allison 2009-05-28 12:11:48 UTC
Just a quick comment. I implemented cli_posix_readlink in the s3 client libraries yesterday and noticed that the readlink/symlink call in s3 smbd does ucs2 text translation to/from the symlink. This is wrong (IMHO) as symlinks should be able to contain any arbitrary binary blobs, but is probably too late to fix. However just an FYI, as the readlink call will return the contents of a link in ucs2 form then the max returned bytes needs to be bigger than PATH_MAX+1 due to the unicode expansion (something I had to deal with in cli_posix_readlink()).
Jeremy.
Comment 12 Jeff Layton 2009-05-28 14:01:24 UTC
I think we're ok here. I made the patch change the MaxDataCount to be equal to the size of the buffer (16k by default, 8k minimum). I don't think that larger size will be a problem.