| Message ID | 20241005160826.20825-1-pali@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | cifs: Improve access without FILE_READ_ATTRIBUTES permission | expand |
The obvious question to check is whether this would lead to any issues if desired_access is not passed in in oparms in any cases (ie if it ends up 0), and also that this would not hurt any cases where we want to keep the handle cached (deferred close) but don't have sufficient permission for it to be usable by the subsequent operation (e.g. revalidate or stat) On Sat, Oct 5, 2024 at 11:10 AM Pali Rohár <pali@kernel.org> wrote: > > Linux SMB client currently is not able to access files for which do not > have FILE_READ_ATTRIBUTES permission. > > For example it is not able to write data into file on SMB server to > which has only write access (no read or read attributes access). And > applications are not able to get result of stat() syscall on such file. > > Test case against Windows SMB server: > > 1) On SMB server prepare file with only GENERIC_WRITE access for Everyone: > ACL:S-1-1-0:ALLOWED/0x0/0x40000000 > > 2) On SMB server remove all access for file's parent directory > > 3) Mount share by Linux SMB client and try to append data to that file: > echo test >> /mnt/share/dir/file > > 4) Try to call: stat /mnt/share/dir/file > > Without this change the write test fails because Linux SMB client is trying > to open SMB path "\dir\file" with GENERIC_WRITE|FILE_READ_ATTRIBUTES. With > this change the test pass as Linux SMB client is not opening file with > FILE_READ_ATTRIBUTES access anymore. > > Similarly without this change the stat test always fails as Linux SMB > client is trying to read attributes via SMB2_OP_QUERY_INFO. With this > change, if SMB2_OP_QUERY_INFO fails then Linux SMB client fallbacks for > reading stat attributes via OPEN with MAXIMUM_ALLOWED access (which will > pass if there is some permission) and OPEN reply will contain attributes > required for stat(). > > Pali Rohár (2): > cifs: Do not issue SMB2 CREATE always with FILE_READ_ATTRIBUTES > cifs: Improve stat() to work also without FILE_READ_ATTRIBUTES > > fs/smb/client/cifspdu.h | 1 + > fs/smb/client/smb2file.c | 1 - > fs/smb/client/smb2glob.h | 1 + > fs/smb/client/smb2inode.c | 71 ++++++++++++++++++++++++++++++++++++++- > 4 files changed, 72 insertions(+), 2 deletions(-) > > -- > 2.20.1 > >
On Saturday 05 October 2024 13:32:12 Steve French wrote: > The obvious question to check is whether this would lead to any issues > if desired_access is not passed in in oparms in any cases (ie if it > ends up 0), This is good point. IIRC if zero value is in OPEN/CREATE desired_access request then SMB server returns STATUS_ACCESS_DENIED. So it needs to be checked that desired_access is filled in all usage correctly. > and also that this would not hurt any cases where we want > to keep the handle cached (deferred close) but don't have sufficient > permission for it to be usable by the subsequent operation (e.g. > revalidate or stat) I see, so the code needs to be properly checked or tested that all these conditions are handled. > On Sat, Oct 5, 2024 at 11:10 AM Pali Rohár <pali@kernel.org> wrote: > > > > Linux SMB client currently is not able to access files for which do not > > have FILE_READ_ATTRIBUTES permission. > > > > For example it is not able to write data into file on SMB server to > > which has only write access (no read or read attributes access). And > > applications are not able to get result of stat() syscall on such file. > > > > Test case against Windows SMB server: > > > > 1) On SMB server prepare file with only GENERIC_WRITE access for Everyone: > > ACL:S-1-1-0:ALLOWED/0x0/0x40000000 > > > > 2) On SMB server remove all access for file's parent directory > > > > 3) Mount share by Linux SMB client and try to append data to that file: > > echo test >> /mnt/share/dir/file > > > > 4) Try to call: stat /mnt/share/dir/file > > > > Without this change the write test fails because Linux SMB client is trying > > to open SMB path "\dir\file" with GENERIC_WRITE|FILE_READ_ATTRIBUTES. With > > this change the test pass as Linux SMB client is not opening file with > > FILE_READ_ATTRIBUTES access anymore. > > > > Similarly without this change the stat test always fails as Linux SMB > > client is trying to read attributes via SMB2_OP_QUERY_INFO. With this > > change, if SMB2_OP_QUERY_INFO fails then Linux SMB client fallbacks for > > reading stat attributes via OPEN with MAXIMUM_ALLOWED access (which will > > pass if there is some permission) and OPEN reply will contain attributes > > required for stat(). > > > > Pali Rohár (2): > > cifs: Do not issue SMB2 CREATE always with FILE_READ_ATTRIBUTES > > cifs: Improve stat() to work also without FILE_READ_ATTRIBUTES > > > > fs/smb/client/cifspdu.h | 1 + > > fs/smb/client/smb2file.c | 1 - > > fs/smb/client/smb2glob.h | 1 + > > fs/smb/client/smb2inode.c | 71 ++++++++++++++++++++++++++++++++++++++- > > 4 files changed, 72 insertions(+), 2 deletions(-) > > > > -- > > 2.20.1 > > > > > > > -- > Thanks, > > Steve
On Saturday 05 October 2024 20:44:53 Pali Rohár wrote: > On Saturday 05 October 2024 13:32:12 Steve French wrote: > > The obvious question to check is whether this would lead to any issues > > if desired_access is not passed in in oparms in any cases (ie if it > > ends up 0), > > This is good point. IIRC if zero value is in OPEN/CREATE desired_access > request then SMB server returns STATUS_ACCESS_DENIED. > > So it needs to be checked that desired_access is filled in all usage > correctly. I have done checks and seems that all callers put some non-zero desired access to smb2_open_file() call. So I think this should not be an issue. > > and also that this would not hurt any cases where we want > > to keep the handle cached (deferred close) but don't have sufficient > > permission for it to be usable by the subsequent operation (e.g. > > revalidate or stat) > > I see, so the code needs to be properly checked or tested that all these > conditions are handled. It looks like that when rdwr_for_fscache is used then proper read/write desired access is asked. During testing I have not spotted issues. Also to note, I checked SMB1 code and it already does not automatically add FILE_READ_ATTRIBUTES to desired access during open. Could you schedule this change for some testing? > > On Sat, Oct 5, 2024 at 11:10 AM Pali Rohár <pali@kernel.org> wrote: > > > > > > Linux SMB client currently is not able to access files for which do not > > > have FILE_READ_ATTRIBUTES permission. > > > > > > For example it is not able to write data into file on SMB server to > > > which has only write access (no read or read attributes access). And > > > applications are not able to get result of stat() syscall on such file. > > > > > > Test case against Windows SMB server: > > > > > > 1) On SMB server prepare file with only GENERIC_WRITE access for Everyone: > > > ACL:S-1-1-0:ALLOWED/0x0/0x40000000 > > > > > > 2) On SMB server remove all access for file's parent directory > > > > > > 3) Mount share by Linux SMB client and try to append data to that file: > > > echo test >> /mnt/share/dir/file > > > > > > 4) Try to call: stat /mnt/share/dir/file > > > > > > Without this change the write test fails because Linux SMB client is trying > > > to open SMB path "\dir\file" with GENERIC_WRITE|FILE_READ_ATTRIBUTES. With > > > this change the test pass as Linux SMB client is not opening file with > > > FILE_READ_ATTRIBUTES access anymore. > > > > > > Similarly without this change the stat test always fails as Linux SMB > > > client is trying to read attributes via SMB2_OP_QUERY_INFO. With this > > > change, if SMB2_OP_QUERY_INFO fails then Linux SMB client fallbacks for > > > reading stat attributes via OPEN with MAXIMUM_ALLOWED access (which will > > > pass if there is some permission) and OPEN reply will contain attributes > > > required for stat(). > > > > > > Pali Rohár (2): > > > cifs: Do not issue SMB2 CREATE always with FILE_READ_ATTRIBUTES > > > cifs: Improve stat() to work also without FILE_READ_ATTRIBUTES > > > > > > fs/smb/client/cifspdu.h | 1 + > > > fs/smb/client/smb2file.c | 1 - > > > fs/smb/client/smb2glob.h | 1 + > > > fs/smb/client/smb2inode.c | 71 ++++++++++++++++++++++++++++++++++++++- > > > 4 files changed, 72 insertions(+), 2 deletions(-) > > > > > > -- > > > 2.20.1 > > > > > > > > > > > > -- > > Thanks, > > > > Steve