search

cifsd-team / ksmbd

ksmbd kernel server(SMB/CIFS server)
152 stars 23 forks source link

Initialize d_info in smb1pdu find_next() #441

Closed xdarklight closed 3 years ago

xdarklight commented 3 years ago

For most errors the find_next() implementation jumps to the err_out label. This unconditionally calls kfree(d_info.smb1_name). Use memset() to zero-out the struct ksmbd_dir_info d_info variable at the beginning of the function so when any error occurs d_info.smb1_name will either be NULL or have a valid pointer in it.

This fixes a crash which was observed in kmem_cache_free_bulk() - presumably because the code was trying to free memory from an address which was just garbage (non-initialized) data from the stack.

Fixes #440 for me

neheb commented 3 years ago

I wonder if memset or initializing with {} is better.

namjaejeon commented 3 years ago

and need to fix it in find_first together.

namjaejeon commented 3 years ago

I wonder if memset or initializing with {} is better.

someone told me the performance of memset is better than {}'s one before.

xdarklight commented 3 years ago

good point @namjaejeon - I'll update this PR

personally I would like to move the discussion about memset vs {} elsewhere because fixing this bug is important (IMO). this bug can be used to remotely crash a system when SMB1 is enabled. sure, SMB1 is not meant to be "secure", but at the same time there shouldn't be any way for it to crash the whole system

namjaejeon commented 3 years ago

@xdarklight Okay. Thanks!

neheb commented 3 years ago

I wonder if memset or initializing with {} is better.

someone told me the performance of memset is better than {}'s one before.

That was me. I don't think I was right. The former does the zeroing in place. The latter makes a function call.

memset is more flexible as {} only zeroes the struct.

Anyway, I agree with @xdarklight . It's beyond the scope of this PR.