Re: [PATCH] drivers/ata: Fix kernel pointer leak

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 2021/09/30 17:54, Sergey Shtylyov wrote:
> On 30.09.2021 5:35, Damien Le Moal wrote:
>> On 2021/09/29 21:16, Guo Zhi wrote:
>>> Pointers should be printed with %p or %px rather than cast to
>>> 'unsigned long' and pinted with %lx
>>
>> s/pinted/printed
>>
>>> Change %lx to %p to print the secured pointer.
>>>
>>> Signed-off-by: Guo Zhi <qtxuning1999@sjtu.edu.cn>
>>> ---
>>>   drivers/ata/pata_atp867x.c | 10 +++++-----
>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/ata/pata_atp867x.c b/drivers/ata/pata_atp867x.c
>>> index 2bc5fc81efe3..c32b95f48e50 100644
>>> --- a/drivers/ata/pata_atp867x.c
>>> +++ b/drivers/ata/pata_atp867x.c
>>> @@ -447,11 +447,11 @@ static int atp867x_ata_pci_sff_init_host(struct ata_host *host)
>>>   #ifdef	ATP867X_DEBUG
>>>   		atp867x_check_ports(ap, i);
>>>   #endif
>>> -		ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
>>> -			(unsigned long)ioaddr->cmd_addr,
>>> -			(unsigned long)ioaddr->ctl_addr);
>>> -		ata_port_desc(ap, "bmdma 0x%lx",
>>> -			(unsigned long)ioaddr->bmdma_addr);
>>> +		ata_port_desc(ap, "cmd 0x%p ctl 0x%p",
>>> +			ioaddr->cmd_addr,
>>> +			ioaddr->ctl_addr);
>>> +		ata_port_desc(ap, "bmdma 0x%p",
>>> +			ioaddr->bmdma_addr);
>>>   
>>>   		mask |= 1 << i;
>>>   	}
>>>
>>
>> Looks OK to me. But please fix the commit title to:
>>
>> "ata: atp867x: Fix pointer value print"
>>
>> "pointer leak" is too scary for what is only a simple printk problem.
> 
>     It's not a simple printk() problem, it's an kernel info leak that he's 
> fixing. But, as I said, this driver doesn't use MMIO, so "leaks" only I/O port 
> addresses.

OK. I interpreted "leak" as memory leak... So the problem is print of pointer
addresses that are unused. But if they are, shouldn't the pointers be NULL ? (I
am absolutely not familiar with this driver, never looked at it).

Guo,

Can you check if the values printed are actually correct and correspond to
resources used by the driver ? If they are not, simply remove the
ata_port_desc() calls.


> 
> MBR, Sergey
> 


-- 
Damien Le Moal
Western Digital Research