On 12. Mar 2024, at 16.43, Chenyuan Yang <chenyuan0y@xxxxxxxxx> wrote: > > Dear Linux Developers for SCSI Driver, > > We are curious about the functionality of `st_setup` > (https://elixir.bootlin.com/linux/latest/source/drivers/scsi/st.c#L4102). > > ``` > static int __init st_setup(char *str) > { > int i, len, ints[5]; > char *stp; > > stp = get_options(str, ARRAY_SIZE(ints), ints); > > if (ints[0] > 0) { > for (i = 0; i < ints[0] && i < ARRAY_SIZE(parms); i++) > if (parms[i].val) > *parms[i].val = ints[i + 1]; > } > ... > } > ``` > > For this function, we are trying to understand how it works but not > sure whether it would be an out-of-bound read. > > The length of both `ints` and `parms` is 5 (the latterdefined at > https://elixir.bootlin.com/linux/latest/source/drivers/scsi/st.c#L125). > Thus, when `ints[0]` is 5, we could assign `ints[5]` > (out-of-bound-read) to `parms[4].val`. Based on our understanding of > the `get_options` function > (https://elixir.bootlin.com/linux/latest/source/lib/cmdline.c#L107), > it could be possible that `ints[0] == 5`, where the first element of > `ints` indicates the number of parsed options. Hence, it is possible > to do > a out-of-bound read once `debug_flag` is enabled (to pass `if > (parms[i].val)`). > I think your analysis is correct and there is a bug. > Please correct us if we miss some key prerequisites for this function > or the data structure. > Thanks in advance! > > Based on our understanding, the possible fix could be > ``` > int i, len, ints[6]; > ``` > which allocates `len(parms) + 1` for `ints`. Yes, this would fix the bug. However, it might be better to define size of ints[] as ARRAY_SIZE(parms)+1 to connect the size directly to the definition of parameters. (The bug applies to the case where st is compiled into the kernel and a list of integers is used to define the options. Not a common case, but a bug should be fixed.) Thanks, Kai
