* [PATCH] x86/ioremap: tighten integer overflow checking
@ 2018-10-25 6:16 Dan Carpenter
2018-10-25 6:28 ` Juergen Gross
2018-10-29 18:34 ` Thomas Gleixner
0 siblings, 2 replies; 4+ messages in thread
From: Dan Carpenter @ 2018-10-25 6:16 UTC (permalink / raw)
To: Dave Hansen
Cc: Andy Lutomirski, Peter Zijlstra, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
The current check is a bit off in the case where "phys_addr + size"
wraps to zero because then "last_addr" is set to ULONG_MAX which is >=
phys_addr.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
arch/x86/mm/ioremap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 5378d10f1d31..ee43df3ebe66 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -146,9 +146,9 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
void __iomem *ret_addr;
/* Don't allow wraparound or zero size */
- last_addr = phys_addr + size - 1;
- if (!size || last_addr < phys_addr)
+ if (!size || phys_addr + size < phys_addr)
return NULL;
+ last_addr = phys_addr + size - 1;
if (!phys_addr_valid(phys_addr)) {
printk(KERN_WARNING "ioremap: invalid physical address %llx\n",
--
2.11.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/ioremap: tighten integer overflow checking
2018-10-25 6:16 [PATCH] x86/ioremap: tighten integer overflow checking Dan Carpenter
@ 2018-10-25 6:28 ` Juergen Gross
2018-10-25 7:08 ` Dan Carpenter
2018-10-29 18:34 ` Thomas Gleixner
1 sibling, 1 reply; 4+ messages in thread
From: Juergen Gross @ 2018-10-25 6:28 UTC (permalink / raw)
To: Dan Carpenter, Dave Hansen
Cc: Andy Lutomirski, Peter Zijlstra, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
On 25/10/2018 08:16, Dan Carpenter wrote:
> The current check is a bit off in the case where "phys_addr + size"
> wraps to zero because then "last_addr" is set to ULONG_MAX which is >=
> phys_addr.
And -2 would be okay?
For 32-bit systems I believe ULONG_MAX is a perfectly valid physical
address.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> arch/x86/mm/ioremap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 5378d10f1d31..ee43df3ebe66 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -146,9 +146,9 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
> void __iomem *ret_addr;
>
> /* Don't allow wraparound or zero size */
> - last_addr = phys_addr + size - 1;
> - if (!size || last_addr < phys_addr)
> + if (!size || phys_addr + size < phys_addr)
> return NULL;
> + last_addr = phys_addr + size - 1;
>
> if (!phys_addr_valid(phys_addr)) {
Wouldn't it make more sense to test last_addr for being a valid physical
address here?
> printk(KERN_WARNING "ioremap: invalid physical address %llx\n",
>
Juergen
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/ioremap: tighten integer overflow checking
2018-10-25 6:28 ` Juergen Gross
@ 2018-10-25 7:08 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2018-10-25 7:08 UTC (permalink / raw)
To: Juergen Gross
Cc: Dave Hansen, Andy Lutomirski, Peter Zijlstra, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
On Thu, Oct 25, 2018 at 08:28:44AM +0200, Juergen Gross wrote:
> On 25/10/2018 08:16, Dan Carpenter wrote:
> > The current check is a bit off in the case where "phys_addr + size"
> > wraps to zero because then "last_addr" is set to ULONG_MAX which is >=
> > phys_addr.
>
> And -2 would be okay?
>
If you see any patches from me it's static checker stuff...
I guess the original is actually fine because the -1 re-wraps back to
what we wanted it to be. We also do verify that size is correct in
__ioremap_check_mem() so I can't see a problem here after all.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/ioremap: tighten integer overflow checking
2018-10-25 6:16 [PATCH] x86/ioremap: tighten integer overflow checking Dan Carpenter
2018-10-25 6:28 ` Juergen Gross
@ 2018-10-29 18:34 ` Thomas Gleixner
1 sibling, 0 replies; 4+ messages in thread
From: Thomas Gleixner @ 2018-10-29 18:34 UTC (permalink / raw)
To: Dan Carpenter
Cc: Dave Hansen, Andy Lutomirski, Peter Zijlstra, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
Dan,
On Thu, 25 Oct 2018, Dan Carpenter wrote:
> The current check is a bit off in the case where "phys_addr + size"
> wraps to zero because then "last_addr" is set to ULONG_MAX which is >=
> phys_addr.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> arch/x86/mm/ioremap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 5378d10f1d31..ee43df3ebe66 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -146,9 +146,9 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
> void __iomem *ret_addr;
>
> /* Don't allow wraparound or zero size */
> - last_addr = phys_addr + size - 1;
> - if (!size || last_addr < phys_addr)
> + if (!size || phys_addr + size < phys_addr)
Assume the following (resource_size_t == u32, which is the case when
CONFIG_PHYS_ADDR_T_64BIT=n):
phys_addr = 0xFFFF0000
size = 0x00010000
sum = 0x00000000 which is < phys_addr
But the existing code does:
last_addr = phys_addr + size - 1
= 0xFFFFFFFF
which is correct. last_addr is the last valid address in the to be remapped
range starting @phys_addr.
Thanks,
tglx
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-10-29 18:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-25 6:16 [PATCH] x86/ioremap: tighten integer overflow checking Dan Carpenter
2018-10-25 6:28 ` Juergen Gross
2018-10-25 7:08 ` Dan Carpenter
2018-10-29 18:34 ` Thomas Gleixner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).