linux-kernel - Re: [PATCH 2/2] ntb_hw_switchtec: Check for alignment of the buffer in mw_set

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d06cf74e-125c-6281-9361-22bac3468251@deltatee.com>
Date:   Mon, 11 Dec 2017 10:14:48 -0700
From:   Logan Gunthorpe <logang@...tatee.com>
To:     Allen Hubbe <Allen.Hubbe@...l.com>, linux-ntb@...glegroups.com,
        linux-kernel@...r.kernel.org
Cc:     'Jon Mason' <jdmason@...zu.us>
Subject: Re: [PATCH 2/2] ntb_hw_switchtec: Check for alignment of the buffer
 in mw_set_trans()

On 11/12/17 07:58 AM, Allen Hubbe wrote:
> !IS_ALIGNED(addr, BIT_ULL(xlate_pos))
> 

Ok, yes, that's much better. I'll change it. Thanks.

>> +		/*
>> +		 * In certain circumstances we can get a buffer that is
>> +		 * not aligned to its size. (Most of the time
>> +		 * dma_alloc_coherent ensures this). This can happen when
>> +		 * using large buffers allocated by the CMA
>> +		 * (see CMA_CONFIG_ALIGNMENT)
>> +		 */
>> +		dev_err(&sndev->stdev->dev,
>> +			"ERROR: Memory window address is not aligned to it's size!\n");
> 
> This would be the only ntb hw driver that prints an error in this situation.  The ntb_mw_get_align() should provide enough information to client drivers to determine the alignment requirements before calling ntb_mw_set_trans().  IMO no need to print here, but let's see what others say.

mw_get_align doesn't communicate the fact that the buffer has to be 
aligned by its size. It may also be that all hardware does not have this 
restriction (ie. if the hardware adds to the base address instead of 
just replacing the lower bits).

There is definitely a need to print this error somewhere as I hit this 
case and it caused very weird behavior. It was a huge pain to debug. 
Also, it's a security issue and huge bug if we end up mapping the memory 
we didn't think we were mapping. I don't think it's a good idea for us 
to require clients to check this as that requires a number of checks and 
a client author may forget to add it to their driver. I'd maybe go with 
a check in ntb_mw_set_trans before calling the driver, but that only 
makes sense if all hardware has the same requirement.

Logan