[PATCH] cifs: create sd context must be a multiple of 8

[PATCH] cifs: create sd context must be a multiple of 8

[Shyam Prasad N]

Hi Steve,

Please review the fix for the bug reported at:
https://bugzilla.kernel.org/show_bug.cgi?id=213927

The issue was misalignment of create context caused by one of our
earlier commit:
commit ea64370bcae126a88cd26a16f1abcc23ab2b9a55 (tag: 5.10-rc6-smb3-fixes-part2)
Author: Ronnie Sahlberg <lsahlber@redhat.com>
Date:   Mon Nov 30 11:29:20 2020 +1000

    cifs: refactor create_sd_buf() and and avoid corrupting the buffer

    When mounting with "idsfromsid" mount option, Azure
    corrupted the owner SIDs due to excessive padding
    caused by placing the owner fields at the end of the
    security descriptor on create.  Placing owners at the
    front of the security descriptor (rather than the end)
    is also safer, as the number of ACEs (that follow it)
    are variable.

    Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
    Suggested-by: Rohith Surabattula <rohiths@microsoft.com>
    CC: Stable <stable@vger.kernel.org> # v5.8
    Signed-off-by: Steve French <stfrench@microsoft.com>

The fix can be found at:
https://github.com/sprasad-microsoft/smb3-kernel-client/pull/4

I think this should be marked for stable as well, with a "fixes" tag.

-- 
Regards,
Shyam

Re: [PATCH] cifs: create sd context must be a multiple of 8

[ronnie sahlberg]

Acked by me.

The entire handling of how we do contexts length and padding is broken I think.
We really should have a model where we add the contexts one at a time,
with the actual length of the context
(without any padding)
and then once we have the list of all contexts we iterate over this
list and adjust the lengths and adding padding where needed,

Currently we add padding very ad-hoc only in the contexts we know
where it might be needed, and this leads to bugs like this, where we
forget.
I think the context handling in negotiate protocol is even worse.


On Thu, Aug 5, 2021 at 3:17 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> Hi Steve,
>
> Please review the fix for the bug reported at:
> https://bugzilla.kernel.org/show_bug.cgi?id=213927
>
> The issue was misalignment of create context caused by one of our
> earlier commit:
> commit ea64370bcae126a88cd26a16f1abcc23ab2b9a55 (tag: 5.10-rc6-smb3-fixes-part2)
> Author: Ronnie Sahlberg <lsahlber@redhat.com>
> Date:   Mon Nov 30 11:29:20 2020 +1000
>
>     cifs: refactor create_sd_buf() and and avoid corrupting the buffer
>
>     When mounting with "idsfromsid" mount option, Azure
>     corrupted the owner SIDs due to excessive padding
>     caused by placing the owner fields at the end of the
>     security descriptor on create.  Placing owners at the
>     front of the security descriptor (rather than the end)
>     is also safer, as the number of ACEs (that follow it)
>     are variable.
>
>     Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
>     Suggested-by: Rohith Surabattula <rohiths@microsoft.com>
>     CC: Stable <stable@vger.kernel.org> # v5.8
>     Signed-off-by: Steve French <stfrench@microsoft.com>
>
> The fix can be found at:
> https://github.com/sprasad-microsoft/smb3-kernel-client/pull/4
>
> I think this should be marked for stable as well, with a "fixes" tag.
>
> --
> Regards,
> Shyam