Re: [v2 PATCH for-4.4 04/16] SUNRPC: fix refcounting problems with auth_gss messages.

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sun, Apr 16, 2017 at 01:34:22PM +0530, Sumit Semwal wrote:
> Hi Greg,
> 
> 
> 
> On Apr 16, 2017 13:29, "Greg KH" <gregkh@linuxfoundation.org> wrote:
> 
>     On Wed, Apr 12, 2017 at 11:13:38PM +0530, Sumit Semwal wrote:
>     > From: NeilBrown <neilb@suse.com>
>     >
>     > [ Upstream commit 1cded9d2974fe4fe339fc0ccd6638b80d465ab2c ]
>     >
>     > There are two problems with refcounting of auth_gss messages.
>     >
>     > First, the reference on the pipe->pipe list (taken by a call
>     > to rpc_queue_upcall()) is not counted.� It seems to be
>     > assumed that a message in pipe->pipe will always also be in
>     > pipe->in_downcall, where it is correctly reference counted.
>     >
>     > However there is no guaranty of this.� I have a report of a
>     > NULL dereferences in rpc_pipe_read() which suggests a msg
>     > that has been freed is still on the pipe->pipe list.
>     >
>     > One way I imagine this might happen is:
>     > - message is queued for uid=U and auth->service=S1
>     > - rpc.gssd reads this message and starts processing.
>     >� �This removes the message from pipe->pipe
>     > - message is queued for uid=U and auth->service=S2
>     > - rpc.gssd replies to the first message. gss_pipe_downcall()
>     >� �calls __gss_find_upcall(pipe, U, NULL) and it finds the
>     >� �*second* message, as new messages are placed at the head
>     >� �of ->in_downcall, and the service type is not checked.
>     > - This second message is removed from ->in_downcall and freed
>     >� �by gss_release_msg() (even though it is still on pipe->pipe)
>     > - rpc.gssd tries to read another message, and dereferences a pointer
>     >� �to this message that has just been freed.
>     >
>     > I fix this by incrementing the reference count before calling
>     > rpc_queue_upcall(), and decrementing it if that fails, or normally in
>     > gss_pipe_destroy_msg().
>     >
>     > It seems strange that the reply doesn't target the message more
>     > precisely, but I don't know all the details.� In any case, I think the
>     > reference counting irregularity became a measureable bug when the
>     > extra arg was added to __gss_find_upcall(), hence the Fixes: line
>     > below.
>     >
>     > The second problem is that if rpc_queue_upcall() fails, the new
>     > message is not freed. gss_alloc_msg() set the ->count to 1,
>     > gss_add_msg() increments this to 2, gss_unhash_msg() decrements to 1,
>     > then the pointer is discarded so the memory never gets freed.
>     >
>     > Fixes: 9130b8dbc6ac ("SUNRPC: allow for upcalls for same uid but
>     different gss service")
>     > Cc: stable@vger.kernel.org
>     > Link: https://bugzilla.opensuse.org/show_bug.cgi?id=1011250
>     > Signed-off-by: NeilBrown <neilb@suse.com>
>     > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>     > Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
>     > ---
>     >� net/sunrpc/auth_gss/auth_gss.c | 7 ++++++-
>     >� 1 file changed, 6 insertions(+), 1 deletion(-)
> 
>     This patch is already in 4.9.2!� Something went really wrong with your
>     patch selection here, why are you sending patches I already have?
> 
> That's true, it's there in 4.9.2, but this patch is marked for 4.4, right?

Doh, that's what I get for working on patches this early, sorry for the
noise...

I'll get to these after this next round of stable kernels go out.

thanks,

greg k-h