lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 20 Oct 2020 07:16:28 +0000
From:   Martijn de Gouw <martijn.de.gouw@...drive-technologies.com>
To:     "J. Bruce Fields" <bfields@...ldses.org>
CC:     Trond Myklebust <trond.myklebust@...merspace.com>,
        Anna Schumaker <anna.schumaker@...app.com>,
        Chuck Lever <chuck.lever@...cle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Arnd Bergmann <arnd@...db.de>, NeilBrown <neilb@...e.de>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Roberto Bergantinos Corpas <rbergant@...hat.com>,
        "open list:NFS, SUNRPC, AND LOCKD CLIENTS" 
        <linux-nfs@...r.kernel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] SUNRPC: fix copying of multiple pages in
 gss_read_proxy_verf()

Hi,

On 20-10-2020 00:04, J. Bruce Fields wrote:
> On Mon, Oct 19, 2020 at 03:46:39PM +0000, Martijn de Gouw wrote:
>> Hi
>>
>> On 19-10-2020 17:23, J. Bruce Fields wrote:
>>> On Mon, Oct 19, 2020 at 01:42:27PM +0200, Martijn de Gouw wrote:
>>>> When the passed token is longer than 4032 bytes, the remaining part
>>>> of the token must be copied from the rqstp->rq_arg.pages. But the
>>>> copy must make sure it happens in a consecutive way.
>>>
>>> Thanks.  Apologies, but I don't immediately see where the copy is
>>> non-consecutive.  What exactly is the bug in the existing code?
>>
>> In the first memcpy 'length' bytes are copied from argv->iobase, but
>> since the header is in front, this never fills the whole first page of
>> in_token->pages.
>>
>> The memcpy in the loop copies the following bytes, but starts writing at
>> the next page of in_token->pages. This leaves the last bytes of page 0
>> unwritten.
>>
>> Next to that, the remaining data is in page 0 of rqstp->rq_arg.pages,
>> not page 1.
> 
> Got it, thanks.  Looks like the culprit might be a patch from a year ago
> from Chuck, 5866efa8cbfb "SUNRPC: Fix svcauth_gss_proxy_init()"?  At
> least, that's the last major patch to touch this code.

I found this issue when setting up NFSv4 with Active Directory as KDC 
and gssproxy. Users with many groups where not able to access the NFS 
shares, while others could access them just fine. During debugging I 
found that the token was not the same on both sides.

I do not have the HW to setup a rdma version of NFSv4, so I'm unable to 
test if it still works via rdma.

Regards, Martijn

> 
> --b.
> 
>>
>> Regards, Martijn
>>
>>>
>>> --b.
>>>
>>>>
>>>> Signed-off-by: Martijn de Gouw <martijn.de.gouw@...drive-technologies.com>
>>>> ---
>>>>    net/sunrpc/auth_gss/svcauth_gss.c | 27 +++++++++++++++++----------
>>>>    1 file changed, 17 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
>>>> index 258b04372f85..bd4678db9d76 100644
>>>> --- a/net/sunrpc/auth_gss/svcauth_gss.c
>>>> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
>>>> @@ -1147,9 +1147,9 @@ static int gss_read_proxy_verf(struct svc_rqst *rqstp,
>>>>    			       struct gssp_in_token *in_token)
>>>>    {
>>>>    	struct kvec *argv = &rqstp->rq_arg.head[0];
>>>> -	unsigned int page_base, length;
>>>> -	int pages, i, res;
>>>> -	size_t inlen;
>>>> +	unsigned int length, pgto_offs, pgfrom_offs;
>>>> +	int pages, i, res, pgto, pgfrom;
>>>> +	size_t inlen, to_offs, from_offs;
>>>>    
>>>>    	res = gss_read_common_verf(gc, argv, authp, in_handle);
>>>>    	if (res)
>>>> @@ -1177,17 +1177,24 @@ static int gss_read_proxy_verf(struct svc_rqst *rqstp,
>>>>    	memcpy(page_address(in_token->pages[0]), argv->iov_base, length);
>>>>    	inlen -= length;
>>>>    
>>>> -	i = 1;
>>>> -	page_base = rqstp->rq_arg.page_base;
>>>> +	to_offs = length;
>>>> +	from_offs = rqstp->rq_arg.page_base;
>>>>    	while (inlen) {
>>>> -		length = min_t(unsigned int, inlen, PAGE_SIZE);
>>>> -		memcpy(page_address(in_token->pages[i]),
>>>> -		       page_address(rqstp->rq_arg.pages[i]) + page_base,
>>>> +		pgto = to_offs >> PAGE_SHIFT;
>>>> +		pgfrom = from_offs >> PAGE_SHIFT;
>>>> +		pgto_offs = to_offs & ~PAGE_MASK;
>>>> +		pgfrom_offs = from_offs & ~PAGE_MASK;
>>>> +
>>>> +		length = min_t(unsigned int, inlen,
>>>> +			 min_t(unsigned int, PAGE_SIZE - pgto_offs,
>>>> +			       PAGE_SIZE - pgfrom_offs));
>>>> +		memcpy(page_address(in_token->pages[pgto]) + pgto_offs,
>>>> +		       page_address(rqstp->rq_arg.pages[pgfrom]) + pgfrom_offs,
>>>>    		       length);
>>>>    
>>>> +		to_offs += length;
>>>> +		from_offs += length;
>>>>    		inlen -= length;
>>>> -		page_base = 0;
>>>> -		i++;
>>>>    	}
>>>>    	return 0;
>>>>    }
>>>> -- 
>>>> 2.20.1
>>
>> -- 
>> Martijn de Gouw
>> Designer
>> Prodrive Technologies
>> Mobile: +31 63 17 76 161
>> Phone:  +31 40 26 76 200

-- 
Martijn de Gouw
Designer
Prodrive Technologies
Mobile: +31 63 17 76 161
Phone:  +31 40 26 76 200

Powered by blists - more mailing lists