| Message ID | 20210929121618.1157415-1-qtxuning1999@sjtu.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | drivers/ata: Fix kernel pointer leak | expand |
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
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.
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
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 >
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
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 >
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
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; }
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(-)