| Message ID | 20230918205209.11709-1-s.shtylyov@omp.ru |
|---|---|
| Headers | show |
| Series | Fix sloppy typing in the area copy | expand |
On 9/18/23 22:52, Sergey Shtylyov wrote: > In cfb_copyarea(), when initializing *unsigned long const* bits_per_line > __u32 typed fb_fix_screeninfo::line_length gets multiplied by 8u -- which > might overflow __u32; multiplying by 8UL instead should fix that... > Also, that bits_per_line constant is used to advance *unsigned* src_idx > and dst_idx variables -- which might be overflowed as well; declaring > them as *unsigned long* should fix that too... > > Found by Linux Verification Center (linuxtesting.org) with the Svace static > analysis tool. > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > Cc: stable@vger.kernel.org > --- > drivers/video/fbdev/core/cfbcopyarea.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/fbdev/core/cfbcopyarea.c b/drivers/video/fbdev/core/cfbcopyarea.c > index 6d4bfeecee35..b67ba69ea2fb 100644 > --- a/drivers/video/fbdev/core/cfbcopyarea.c > +++ b/drivers/video/fbdev/core/cfbcopyarea.c > @@ -382,10 +382,11 @@ void cfb_copyarea(struct fb_info *p, const struct fb_copyarea *area) > { > u32 dx = area->dx, dy = area->dy, sx = area->sx, sy = area->sy; > u32 height = area->height, width = area->width; > - unsigned long const bits_per_line = p->fix.line_length*8u; > + unsigned long const bits_per_line = p->fix.line_length * 8UL; you wrote: > __u32 typed fb_fix_screeninfo::line_length gets multiplied by 8u -- which > might overflow __u32; multiplying by 8UL instead should fix that... This would only be true on 64-bit CPUs, where unsigned long is 64 bits, while on 32-bit CPUs, it's still 32 bits (same as _u32). Instead we could make bits_per_line __u32 (or unsigned int) too. > unsigned long __iomem *base = NULL; > int bits = BITS_PER_LONG, bytes = bits >> 3; > - unsigned dst_idx = 0, src_idx = 0, rev_copy = 0; > + unsigned long dst_idx = 0, src_idx = 0; An "unsigned int" can address at least up to 4GB, which is fully sufficent here. So, both patches don't have any real effect. NAK. Helge
Hello! On 9/19/23 10:05 AM, Helge Deller wrote: >> In cfb_copyarea(), when initializing *unsigned long const* bits_per_line >> __u32 typed fb_fix_screeninfo::line_length gets multiplied by 8u -- which >> might overflow __u32; multiplying by 8UL instead should fix that... >> Also, that bits_per_line constant is used to advance *unsigned* src_idx >> and dst_idx variables -- which might be overflowed as well; declaring >> them as *unsigned long* should fix that too... >> >> Found by Linux Verification Center (linuxtesting.org) with the Svace static >> analysis tool. >> >> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> Cc: stable@vger.kernel.org >> --- >> drivers/video/fbdev/core/cfbcopyarea.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/video/fbdev/core/cfbcopyarea.c b/drivers/video/fbdev/core/cfbcopyarea.c >> index 6d4bfeecee35..b67ba69ea2fb 100644 >> --- a/drivers/video/fbdev/core/cfbcopyarea.c >> +++ b/drivers/video/fbdev/core/cfbcopyarea.c >> @@ -382,10 +382,11 @@ void cfb_copyarea(struct fb_info *p, const struct fb_copyarea *area) >> { >> u32 dx = area->dx, dy = area->dy, sx = area->sx, sy = area->sy; >> u32 height = area->height, width = area->width; >> - unsigned long const bits_per_line = p->fix.line_length*8u; >> + unsigned long const bits_per_line = p->fix.line_length * 8UL; > > you wrote: >> __u32 typed fb_fix_screeninfo::line_length gets multiplied by 8u -- which >> might overflow __u32; multiplying by 8UL instead should fix that... > > This would only be true on 64-bit CPUs, where unsigned long is 64 bits, Right, Svace was run with the arm64 and x86_64 configs -- and I forgot to make the emphasis on the 64-bit specifics here... > while on 32-bit CPUs, it's still 32 bits (same as _u32). Yes, indeed. That *unsigned long const* doesn't seem justified at all then... > Instead we could make bits_per_line __u32 (or unsigned int) too. Yes. Will you accept such a patch? :-) >> unsigned long __iomem *base = NULL; >> int bits = BITS_PER_LONG, bytes = bits >> 3; >> - unsigned dst_idx = 0, src_idx = 0, rev_copy = 0; >> + unsigned long dst_idx = 0, src_idx = 0; > > An "unsigned int" can address at least up to 4GB, which is fully sufficent here. Good to know! :-) > So, both patches don't have any real effect. > NAK. Thanks for your time! > Helge MBR, Sergey
On 9/19/23 20:55, Sergey Shtylyov wrote: > On 9/19/23 10:05 AM, Helge Deller wrote: >>> In cfb_copyarea(), when initializing *unsigned long const* bits_per_line >>> __u32 typed fb_fix_screeninfo::line_length gets multiplied by 8u -- which >>> might overflow __u32; multiplying by 8UL instead should fix that... >>> Also, that bits_per_line constant is used to advance *unsigned* src_idx >>> and dst_idx variables -- which might be overflowed as well; declaring >>> them as *unsigned long* should fix that too... >>> >>> Found by Linux Verification Center (linuxtesting.org) with the Svace static >>> analysis tool. >>> >>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>> Cc: stable@vger.kernel.org >>> --- >>> drivers/video/fbdev/core/cfbcopyarea.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/video/fbdev/core/cfbcopyarea.c b/drivers/video/fbdev/core/cfbcopyarea.c >>> index 6d4bfeecee35..b67ba69ea2fb 100644 >>> --- a/drivers/video/fbdev/core/cfbcopyarea.c >>> +++ b/drivers/video/fbdev/core/cfbcopyarea.c >>> @@ -382,10 +382,11 @@ void cfb_copyarea(struct fb_info *p, const struct fb_copyarea *area) >>> { >>> u32 dx = area->dx, dy = area->dy, sx = area->sx, sy = area->sy; >>> u32 height = area->height, width = area->width; >>> - unsigned long const bits_per_line = p->fix.line_length*8u; >>> + unsigned long const bits_per_line = p->fix.line_length * 8UL; >> >> you wrote: >>> __u32 typed fb_fix_screeninfo::line_length gets multiplied by 8u -- which >>> might overflow __u32; multiplying by 8UL instead should fix that... >> >> This would only be true on 64-bit CPUs, where unsigned long is 64 bits, > > Right, Svace was run with the arm64 and x86_64 configs -- and I forgot > to make the emphasis on the 64-bit specifics here... > >> while on 32-bit CPUs, it's still 32 bits (same as _u32). > > Yes, indeed. That *unsigned long const* doesn't seem justified > at all then... > >> Instead we could make bits_per_line __u32 (or unsigned int) too. > > Yes. Will you accept such a patch? :-) Sure. Helge
Here are 2 patches against the 'master' branch of Linus' 'linux.git' repo... In {cfb|sys}_copyarea(), when initializing *unsigned long const* bits_per_line __u32 typed fb_fix_screeninfo::line_length gets multiplied by 8u which might overflow __u32; this whole *struct* fb_fix_screeninfo seems to come from userland (as it's declared in the UAPI header)... Also, that bits_per_line constant is used to advance *unsigned* src_idx and dst_idx lccal variables which might be overflowed as well... These overflow possibilities were there from the very begining of the source files in question, so I decided to just CC stable@vger.kernel.org, without the Fixes tags... Sergey Shtylyov (2): video: fbdev: core: cfbcopyarea: fix sloppy typing video: fbdev: core: syscopyarea: fix sloppy typing drivers/video/fbdev/core/cfbcopyarea.c | 5 +++-- drivers/video/fbdev/core/syscopyarea.c | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-)