1

The code in Linux kernel (maybe a driver):

https://us.codeaurora.org/cgit/quic/la/kernel/msm/tree/drivers/media/platform/msm/camera_v2/isp/msm_isp_util.c?id=38110df3021daf7740018f4b5cc61423c7382aac

checks the size of *data_ptr

sizeof(*data_ptr)

like this:

uint32_t *data_ptr = cfg_data +
        reg_cfg_cmd->u.rw_info.cmd_data_offset/4;

    if ((UINT_MAX - sizeof(*data_ptr) <
                reg_cfg_cmd->u.rw_info.reg_offset) ||
                (resource_size(vfe_dev->vfe_mem) <
                reg_cfg_cmd->u.rw_info.reg_offset +
                sizeof(*data_ptr))) {
                    pr_err("%s: VFE_WRITE_MB: Invalid length\n", __func__);
                    return -EINVAL;
                }

Is the size of

uint32_t *data_ptr

undetermined? It seems that it should always be 4 bytes.

Updated:

If so, what is the meaning of

UINT_MAX - sizeof(*data_ptr)

?

Actually it is a security check, and a vulnerability occurs here. The code is later patched in:

https://us.codeaurora.org/cgit/quic/la//kernel/msm/commit/?id=8ad163e831a2b2c30551edb360f168a604cdb0bb

WindChaser
  • 960
  • 1
  • 10
  • 30
  • 4
    Yes `sizeof(*data_ptr)` should always be 4. But it's better to use `sizeof` rather than hard code to 4. For example, if the type of `data_ptr` changes then the code that uses `sizeof` would work without change but not if it were replaced with a hard coded value. – kaylum May 20 '16 at 05:10
  • Related: http://stackoverflow.com/questions/399003/is-the-sizeofsome-pointer-always-equal-to-four?rq=1 – SKD May 20 '16 at 05:26
  • The patch makes the code worse by introducing code duplication (`uint32_t` appears twice and so the error check may get out of sync with the code if someone changes the data type). Seems like the patch has the right idea of hoisting the error checking to a different place, but it should not have made this particular change to the `sizeof` expression. Also the whole thing is pretty difficult to read, personally I would lay it out so that the checks can all be validated at a glance by a reader. – M.M May 20 '16 at 06:19
  • That's one ugly piece of... if statement right there. Must be the Linux kernel indeed. – Lundin May 20 '16 at 06:42

4 Answers4

2

No, it may not be four bytes. A byte in standard (i.e., ISO) C is not necessarily eight bits. Standards will often use the term "octet" when referring to a specifically-eight-bit item.

Rather a byte is the minimally sized natural data element. If that's a 16-bit data type, then the size of int32_t will be two rather than four.

Given the large variety of architectures Linux and its ilk run on, you should probably allow for variations such as that. Given the possibility that C code often finds its way into totally different systems, it's usually better to have portable code if possible (especially if it costs you nothing).

paxdiablo
  • 854,327
  • 234
  • 1,573
  • 1,953
  • Given that POSIX mandates 8 bit bytes I'm not entirely sure that this is the case. Sure, you can build the userland with a different compiler, but I suspect that the kernel/user interface would get pretty messed up. – Matteo Italia May 20 '16 at 05:52
  • @MatteoItalia, Linux is not POSIX-compliant but I'll clarify that I'm talking about the *real* C standard :-) – paxdiablo May 20 '16 at 05:58
  • The purpose of the stdint types is to provide sane alternatives to the native primitive data types. I don't think it is meaningful to assume that a byte will be anything but 8 bits. Rather, I would strongly encourage everyone to write code that _will_ mysteriously break or crash when executed on some oddball DSP system, where a byte is not 8 bits. You will be doing humanity a favour, by discouraging the use of 1980s crappy DSP architectures. – Lundin May 20 '16 at 06:46
  • 1
    Anyway, this is not why the sizeof operator is there... it is just there to provide self-documenting code instead of "magic numbers". – Lundin May 20 '16 at 06:49
  • @Lundin, that's one viewpoint. Myself, I long for the days when we ditch the weird UTF-8 encoding standard and chars are the full 32-bits wide. When that day comes, all my code will still work. Well, as much as it *ever* worked :-) But, seriously, you're correct in one way. If you're only interested in targeting specific devices, you *can* make assumptions about them. It's all down to the tradeoff between portability and ease of development. – paxdiablo May 20 '16 at 06:52
1

It's not undetermined, it's indeed four bytes. It's however clearer in the code to read sizeof(*data_ptr) rather than 4, since by seeing only the number the reader might be left wondering where the 4 is coming from. See this wikipedia article for a discussion on the topic of magic constants.

Frederik Deweerdt
  • 4,943
  • 2
  • 29
  • 31
1

I believe this is caused by portability: always avoid to use hard coded values. Sizeof() works during compilation, so there is no overhead during execution.

Marcin Kajzler
  • 2,698
  • 1
  • 9
  • 7
0

sizeof(*data_ptr) is the size of the dereferenced pointer. I would venture a guess that uint32_t is an unsigned 32-bit int, which is 4 bytes. sizeof(uint32_t*) gives you the size of the pointer, which depends on your architecture (4 bytes for 32-bit systems, 8 bytes for 64-bit systems).

yano
  • 4,827
  • 2
  • 23
  • 35
  • 1
    `uint32_t` is defined by the C Standard. It is an unsigned 32-bit int (but not necessarily 4 bytes , since some systems have byte sizes other than 8). – M.M May 20 '16 at 06:16
  • @M.M Interesting.. never heard of that – yano May 20 '16 at 14:45
  • @M.M In my experience a byte has always been 8 bits. I found this, and the convo suggests only old legacy systems had different byte sizes: http://stackoverflow.com/questions/13615764/is-a-byte-always-8-bits . I also found this which says the much newer PCIe has a 10-bit byte: https://www.quora.com/Why-is-one-byte-8-bits-even-on-a-16-or-32-bit-machine . Just curious if you had any experience/examples of cases where a byte != 8 bits. Would this apply to mostly embedded chips these days? – yano May 20 '16 at 15:34