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: <2ec869d2-caca-e283-b7b5-a28b5908a4db@deltatee.com>
Date:   Mon, 11 Dec 2017 12:28:34 -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 12:17 PM, Allen Hubbe wrote:
> From: Logan Gunthorpe
> 
>> mw_get_align doesn't communicate the fact that the buffer has to be
>> aligned by its size.
> 
> Is that not the purpose of the addr_align out parameter of ntb_mw_get_align()?

addr_align provides the minimum alignment required by the device but it 
has no idea how big a buffer the caller is trying to create so it can't 
express that it needs to be aligned by its size.

To be clear, the minimum alignment the Switchtec device requires is 4KB 
so it will return 4k in addr_align. Thus, if you have a 4KB buffer it 
may be aligned to 4KB. But if you have a 1MB buffer it must be aligned 
to the nearest 1M.

>> 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.
> 
> Of course the driver should validate its parameters not allow bad mappings.  I was only commenting on the dev_err() message to the console.

Ok. I still feel like it would be difficult to debug if ntb_transport 
simply was unable to establish a connection without some message in 
dmesg telling the user why.

Also, keep in mind this is a somewhat unusual occurrence. In most cases 
dma_alloc_coherent() always provides a buffer that is aligned to it's 
size. It's just that the CMA (if used) provides a tunable config option 
which allows for larger buffers to not be aligned to their size.

Logan