4 ms·

Sorry for being a party pooper, but it didn't take me 5 minutes to find an integer overflow in this code (which I've never seen before), as of commit 2443ff581c

Sorry for being a party pooper, but it didn't take me 5 minutes to find an integer overflow in this code (which I've never seen before), as of commit 2443ff581ccd. The public function nsfb_set_geometry() takes "width" and "height" as "int" values. Assume those are positive. Then we pass them to nsfb->surface_rtns->geometry(). Assume our surface is implemented by "surface/ram.c"; thus the call is made to ram_set_geometry(). There we store the passed-in "int" params into fields of "nsfb" (also ints). Then we do /* reallocate surface memory if necessary */ endsize = (nsfb->width * nsfb->height * nsfb->bpp) / 8; Unchecked multiplication between signed integers (nsfb->width * nsfb->height); not only can it overflow and yield a bogus result, if that happens, it's even undefined behavior. It's naive code.

jcelerier 2y ago
I mean the integer overflow is the least of your problems. If you try to create a 64000*64000 texture most drivers are going to bark at you anyways in the best case.
- snvzz 2y ago
  >If you try to create a 64000*64000 texture Huge texture was a quite famous Linux NVIDIA driver vulnerability.
kragen 2y ago
It sounds like your concern is that if the caller of the public function nsfb_set_geometry() passes in certain arguments, they can invoke undefined behavior, because the function doesn't correctly handle the case where the desired framebuffer is larger than 256 mebibytes (assuming 32-bit ints), and it may suffer an integer arithmetic overflow. That doesn't seem very surprising; nearly any function in C can be crashed by passing it invalid arguments. For example, printf((char)37), free((char)main), or memcpy(argv[0], argv[1], 10485760). Perhaps your concern is that the 256-mebibyte limitation isn't documented? libnsfb in general has very little documentation; this is the only documentation I see for nsfb_set_geometry(): /** Alter the geometry of a surface * * @param nsfb The context to alter. * @param width The new display width. * @param height The new display height. * @param format The desired surface format. */ int nsfb_set_geometry(nsfb_t *nsfb, int width, int height, enum nsfb_format_e format); The README says: API documentation ----------------- Currently, there is none. However, the code is well commented and the public API may be found in the "include" directory. The testcase sources may also be of use in working out how to use it. So I would say that, if you're concerned about documentation, there are much greater deficiencies in documentation than the documentation of this particular limitation. Perhaps your concern is that 256 mebibytes is actually a reasonable size for a framebuffer, not an invalid argument? With the 32-bit formats all modern displays seem to use, that would be 8192 × 8192. That seems like a colorable argument; I've worked with some images larger than that since last millennium. But it still seems serviceable for most purposes. With 16-bit ints, it could fail if the framebuffer was larger than 4096 bytes, which seems like a more serious problem, but I don't know if libnsfb can be built on 16-bit and 8-bit platforms.
- kragen 2y ago
  Sorry, that's printf((char*)37) and free((char*)main).
tightbookkeeper 2y ago
Do you prove that every line of arithmetic in your program will not overflow for all possible inputs? Are you aware that no screen is 64k x 64k?
- teo_zero 2y ago
  > Do you prove that every line of arithmetic in your program will not overflow for all possible inputs? If inputs come from outside, a vehement Yes!
  - cryptonector 2y ago
    In this particular case they wouldn't. But yes, C is a problem.
    - tightbookkeeper 2y ago
      It’s a register based computer problem, not a C problem.
      - fjasdyfs 2y ago
        Not checking for overflow is a developer problem
        tightbookkeeper 2y ago
        Do you suggest branching after every operation? a = b + c if err { // … }
        teo_zero 2y ago
        Please note that you should ensure that overflow doesn't happen, not detect when it happens. Once you let it happen, it's undefined behavior. But you don't need to check each operation to ensure that none of them overflow. If you know that b and c are supposed to be bounded between -10 and +10, for example, the above line can't overflow. So just check that your supposition holds. In most cases, that boils down to a check on the inputs at the entry of the function.
CyberDildonics 2y ago
These are being passed by whoever is using the library. They should be checked before being passed to the function.

jcelerier 2y ago

I mean the integer overflow is the least of your problems. If you try to create a 64000*64000 texture most drivers are going to bark at you anyways in the best case.

snvzz 2y ago
>If you try to create a 64000*64000 texture Huge texture was a quite famous Linux NVIDIA driver vulnerability.

kragen 2y ago

It sounds like your concern is that if the caller of the public function nsfb_set_geometry() passes in certain arguments, they can invoke undefined behavior, because the function doesn't correctly handle the case where the desired framebuffer is larger than 256 mebibytes (assuming 32-bit ints), and it may suffer an integer arithmetic overflow. That doesn't seem very surprising; nearly any function in C can be crashed by passing it invalid arguments. For example, printf((char)37), free((char)main), or memcpy(argv[0], argv[1], 10485760). Perhaps your concern is that the 256-mebibyte limitation isn't documented? libnsfb in general has very little documentation; this is the only documentation I see for nsfb_set_geometry(): /** Alter the geometry of a surface * * @param nsfb The context to alter. * @param width The new display width. * @param height The new display height. * @param format The desired surface format. */ int nsfb_set_geometry(nsfb_t *nsfb, int width, int height, enum nsfb_format_e format); The README says: API documentation ----------------- Currently, there is none. However, the code is well commented and the public API may be found in the "include" directory. The testcase sources may also be of use in working out how to use it. So I would say that, if you're concerned about documentation, there are much greater deficiencies in documentation than the documentation of this particular limitation. Perhaps your concern is that 256 mebibytes is actually a reasonable size for a framebuffer, not an invalid argument? With the 32-bit formats all modern displays seem to use, that would be 8192 × 8192. That seems like a colorable argument; I've worked with some images larger than that since last millennium. But it still seems serviceable for most purposes. With 16-bit ints, it could fail if the framebuffer was larger than 4096 bytes, which seems like a more serious problem, but I don't know if libnsfb can be built on 16-bit and 8-bit platforms.

kragen 2y ago
Sorry, that's printf((char*)37) and free((char*)main).

tightbookkeeper 2y ago

Do you prove that every line of arithmetic in your program will not overflow for all possible inputs? Are you aware that no screen is 64k x 64k?

teo_zero 2y ago
> Do you prove that every line of arithmetic in your program will not overflow for all possible inputs? If inputs come from outside, a vehement Yes!
- cryptonector 2y ago
  In this particular case they wouldn't. But yes, C is a problem.
  - tightbookkeeper 2y ago
    It’s a register based computer problem, not a C problem.
    - fjasdyfs 2y ago
      Not checking for overflow is a developer problem
      - tightbookkeeper 2y ago
        Do you suggest branching after every operation? a = b + c if err { // … }
        teo_zero 2y ago
        Please note that you should ensure that overflow doesn't happen, not detect when it happens. Once you let it happen, it's undefined behavior. But you don't need to check each operation to ensure that none of them overflow. If you know that b and c are supposed to be bounded between -10 and +10, for example, the above line can't overflow. So just check that your supposition holds. In most cases, that boils down to a check on the inputs at the entry of the function.

CyberDildonics 2y ago

These are being passed by whoever is using the library. They should be checked before being passed to the function.