| Message ID | 3727e0b4-3958-283f-c0cf-6b8b898ab018@omp.ru |
|---|---|
| State | New |
| Headers | show |
| Series | ata: libata-core: fix NULL pointer deref in ata_host_alloc_pinfo() | expand |
On 5/21/22 05:53, Sergey Shtylyov wrote: > In an unlikely (and probably wrong?) case that the 'ppi' parameter of > ata_host_alloc_pinfo() points to an array starting with a NULL pointer, > there's going to be a kernel oops as the 'pi' local variable won't get > reassigned from the initial value of NULL. Assign &ata_dummy_port_info > to 'pi' at the start of the *for* loop instead to fix this kernel oops > for good... > > Found by Linux Verification Center (linuxtesting.org) with the SVACE static > analysis tool. > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > --- > This patch is against the 'for-next' branch of Damien's 'libata.git' repo. > > drivers/ata/libata-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Index: libata/drivers/ata/libata-core.c > =================================================================== > --- libata.orig/drivers/ata/libata-core.c > +++ libata/drivers/ata/libata-core.c > @@ -5470,7 +5470,7 @@ struct ata_host *ata_host_alloc_pinfo(st > if (!host) > return NULL; > > - for (i = 0, j = 0, pi = NULL; i < host->n_ports; i++) { > + for (i = 0, j = 0, pi = &ata_dummy_port_info; i < host->n_ports; i++) { > struct ata_port *ap = host->ports[i]; > > if (ppi[j]) I had a fight with this one a while back as the build bot was complaining about this a while back. pi cannot be null in this case, but silencing warnings is good. So OK. Just one nit: please move the initialization of pi to its declaration to avoid the overly long for line.
Hello! On 5/21/22 2:45 AM, Damien Le Moal wrote: >> In an unlikely (and probably wrong?) case that the 'ppi' parameter of >> ata_host_alloc_pinfo() points to an array starting with a NULL pointer, >> there's going to be a kernel oops as the 'pi' local variable won't get >> reassigned from the initial value of NULL. Assign &ata_dummy_port_info >> to 'pi' at the start of the *for* loop instead to fix this kernel oops >> for good... >> >> Found by Linux Verification Center (linuxtesting.org) with the SVACE static >> analysis tool. >> >> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> >> --- >> This patch is against the 'for-next' branch of Damien's 'libata.git' repo. >> >> drivers/ata/libata-core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> Index: libata/drivers/ata/libata-core.c >> =================================================================== >> --- libata.orig/drivers/ata/libata-core.c >> +++ libata/drivers/ata/libata-core.c >> @@ -5470,7 +5470,7 @@ struct ata_host *ata_host_alloc_pinfo(st >> if (!host) >> return NULL; >> >> - for (i = 0, j = 0, pi = NULL; i < host->n_ports; i++) { >> + for (i = 0, j = 0, pi = &ata_dummy_port_info; i < host->n_ports; i++) { >> struct ata_port *ap = host->ports[i]; >> >> if (ppi[j]) > > I had a fight with this one a while back as the build bot was complaining > about this a while back. Hm, what exact tool was complaining? > pi cannot be null in this case, but silencing > warnings is good. So OK. At least it shouldn't be NULL with a tested driver... I found one driver (pata_cs5520) that sets the port info array entries to &ata_dummy_port_info on disabled ports, hence was my idea to also use it... > Just one nit: please move the initialization of pi to its declaration to > avoid the overly long for line. It's not _overly_ long but OK. :-) MBR, Sergey
On 5/22/22 05:00, Sergey Shtylyov wrote: > Hello! > > On 5/21/22 2:45 AM, Damien Le Moal wrote: > >>> In an unlikely (and probably wrong?) case that the 'ppi' parameter of >>> ata_host_alloc_pinfo() points to an array starting with a NULL pointer, >>> there's going to be a kernel oops as the 'pi' local variable won't get >>> reassigned from the initial value of NULL. Assign &ata_dummy_port_info >>> to 'pi' at the start of the *for* loop instead to fix this kernel oops >>> for good... >>> >>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static >>> analysis tool. >>> >>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>> >>> --- >>> This patch is against the 'for-next' branch of Damien's 'libata.git' repo. >>> >>> drivers/ata/libata-core.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> Index: libata/drivers/ata/libata-core.c >>> =================================================================== >>> --- libata.orig/drivers/ata/libata-core.c >>> +++ libata/drivers/ata/libata-core.c >>> @@ -5470,7 +5470,7 @@ struct ata_host *ata_host_alloc_pinfo(st >>> if (!host) >>> return NULL; >>> >>> - for (i = 0, j = 0, pi = NULL; i < host->n_ports; i++) { >>> + for (i = 0, j = 0, pi = &ata_dummy_port_info; i < host->n_ports; i++) { >>> struct ata_port *ap = host->ports[i]; >>> >>> if (ppi[j]) >> >> I had a fight with this one a while back as the build bot was complaining >> about this a while back. > > Hm, what exact tool was complaining? It was the kernel 0-day build bot for non-x86 arch, can't remember which one. > >> pi cannot be null in this case, but silencing >> warnings is good. So OK. > > At least it shouldn't be NULL with a tested driver... I found one driver (pata_cs5520) > that sets the port info array entries to &ata_dummy_port_info on disabled ports, hence > was my idea to also use it... > >> Just one nit: please move the initialization of pi to its declaration to >> avoid the overly long for line. > > It's not _overly_ long but OK. :-) > > MBR, Sergey
Index: libata/drivers/ata/libata-core.c =================================================================== --- libata.orig/drivers/ata/libata-core.c +++ libata/drivers/ata/libata-core.c @@ -5470,7 +5470,7 @@ struct ata_host *ata_host_alloc_pinfo(st if (!host) return NULL; - for (i = 0, j = 0, pi = NULL; i < host->n_ports; i++) { + for (i = 0, j = 0, pi = &ata_dummy_port_info; i < host->n_ports; i++) { struct ata_port *ap = host->ports[i]; if (ppi[j])
In an unlikely (and probably wrong?) case that the 'ppi' parameter of ata_host_alloc_pinfo() points to an array starting with a NULL pointer, there's going to be a kernel oops as the 'pi' local variable won't get reassigned from the initial value of NULL. Assign &ata_dummy_port_info to 'pi' at the start of the *for* loop instead to fix this kernel oops for good... Found by Linux Verification Center (linuxtesting.org) with the SVACE static analysis tool. Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> --- This patch is against the 'for-next' branch of Damien's 'libata.git' repo. drivers/ata/libata-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)