"On Thu, Jun 7, 2018 at 3:26 AM, Qu Wenruo <quwenruo.btrfs@xxxxxxx> wrote: > On 2018年06月07日 15:19, james harvey wrote: >> On Thu, Jun 7, 2018 at 12:44 AM, Qu Wenruo <quwenruo.btrfs@xxxxxxx> wrote: >>> On 2018年06月07日 11:33, james harvey wrote: >>>> * btrfs_item_key_to_cpu sets key to (10955960320, BTRFS_EXTENT_ITEM_KEY, 4096) >>>> !!! Bonus bug, LEFT FOR READER. Why is this item #197, 5 items before the 203 >>>> given? I think no bounds checking causes a buffer over-read here. >>>> btrfs_item_key_to_cpu() calls btrfs_item_key(), which uses the macro >>>> read_eb_member() to call read_extent_buffer() which memcpy's using an out >>>> of range index, at least for this leaf. >>> >>> Here the key you got could be garbage. >>> Either the leaf has enough free space, then you got pending zero into >>> the key, or the leaf is full, you got some item data into the key. >>> >>> Either way, the key should be read/trusted at all. >> >> Is the group's preference to leave things like bounds checking to the >> caller? > > The problem here is, btrfs_search_slot() is not only called for > read_only search, but also for insert. > > For insert usage, such returned path is completely valid and in fact > could be pretty useful. I don't mean for btrfs_search_slot() behavior to change, since it is called for inserting. I mean btrfs_item_key_to_cpu() behavior could change. >> (I get the merit to that, avoiding redundant checks.) My >> normal style would be to have read_extent_buffer() fail if start + len >> is out of bounds. > > In this case, it's not out-of-boundary at all, and the invalid key you > read out if from the last item, which could contain a valid key. > > For leaf, btrfs puts btrfs items pointers at the head, and the items > data at the tail. > > Thus for this case, your invalid key slot is just in the middle of the > leaf, making read_extent_buffer() unable to detect anything wrong. > > Thanks, > Qu You're right. read_extent_buffer() doesn't have the information to look at this. And, "buffer over-read error" was probably the wrong term. I mean in the sense that it has 203 valid values, and is being asked for the 204'th item. btrfs_item_key() could error on (nr > btrfs_header_nritems(eb)) This defensive programming would cause redundant checks, if the caller checks this too as they are supposed to, so I understand if the group doesn't swing that far on defensive programming. Although, perhaps in such situation, btrfs_item_key() could automatically call btrfs_next_leaf(). I'm thinking this might not be desired behavior everywhere, but just in case it is, mentioning it because then btrfs_item_key() could handle all of this, not allowing the caller to shoot their own foot, without redundant checks. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
