[PATCH] drivers/ata: Fix kernel pointer leak

[PATCH] drivers/ata: Fix kernel pointer leak

[Guo Zhi]

Pointers should be printed with %p or %px rather than cast to
'unsigned long' and pinted with %lx
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;
 	}
-- 
2.33.0

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

[Sergey Shtylyov]

On 29.09.2021 15:16, Guo Zhi wrote:

    I'd recommend the subject prefix to be just "pata_atp867x: "...

> Pointers should be printed with %p or %px rather than cast to
> 'unsigned long' and pinted with %lx

    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,

    This line shouldn't be broken up, it's not long at all.

> +			ioaddr->ctl_addr);
> +		ata_port_desc(ap, "bmdma 0x%p",
> +			ioaddr->bmdma_addr);

    Hmm, I've looked at the driver and got an imperession it only uses the I/O 
ports, not MMIO...

[...]

MBR, Sergey

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

[Damien Le Moal]

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.

-- 
Damien Le Moal
Western Digital Research

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

[Guo Zhi]

On 2021/9/30 10: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.
>
I will send a V2 patch.

thanks.

Guo

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

[Damien Le Moal]

On 2021/09/30 11:44, Guo Zhi wrote:
> On 2021/9/30 10: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"

Make that "ata: atp867x: Cleanup pointer value print"

Since this is actually not fixing any problem at all. No need to have this patch
being automatically picked-up for backporting to stable.

>>
>> "pointer leak" is too scary for what is only a simple printk problem.
>>
> I will send a V2 patch.
> 
> thanks.
> 
> Guo
> 


-- 
Damien Le Moal
Western Digital Research

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

[Sergey Shtylyov]

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.

MBR, Sergey

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

[Damien Le Moal]

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

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

[Sergey Shtylyov]

On 10/1/21 4:18 AM, Damien Le Moal 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).

  They are used to map the I/O parts, so the driver can use ioread*()/iowrite()*.

[...]


MBR, Sergey