Hello!

On 9/19/23 10:05 AM, Helge Deller wrote:

quoted

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 <redacted>
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:

quoted

__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? :-)

quoted

      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